-
Notifications
You must be signed in to change notification settings - Fork 617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial rules for running clang-format on the code base #409
Add initial rules for running clang-format on the code base #409
Conversation
Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>
LGTM, but it's hard to judge whether this is the right configuration without seeing what the diffs look like when this is applied to the project. When I clang-format-ified OIIO, I went through a number of iterations of this config before it settled down, and only then did I make the big commits of all the source code post-reformatting. |
Let's don't rush into this, I'd like to iterate on it until we're sure that
the results are really worth it. I anticipate annoying a lot of people at
ILM if we stomp on this revision history of Imath, we need to be certain
it's a good thing. Happy to discuss it further, but I'd suggest holding off
on this until after SIGGRAPH.
…On Wed, Jun 26, 2019 at 1:44 PM Larry Gritz ***@***.***> wrote:
LGTM, but it's hard to judge whether this is the right configuration
without seeing what the diffs look like when this is applied to the project.
When I clang-format-ified OIIO, I went through a number of iterations of
this config before it settled down, and only then did I make the big
commits of all the source code post-reformatting.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#409>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFC3DGLAAXVRUJPN6XTU7CTP4PIKNANCNFSM4H3VPT3A>
.
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
And about the space after the between the function name and parenthesis, as
in "foo (a, b, c)" that's Florian's formatting style, I believe he was the
origin of that convention. I've come to prefer it, but personal preference
aside, I'd tend to favor retaining it for that reason alone.
…On Wed, Jun 26, 2019 at 1:57 PM Cary Phillips ***@***.***> wrote:
Let's don't rush into this, I'd like to iterate on it until we're sure
that the results are really worth it. I anticipate annoying a lot of people
at ILM if we stomp on this revision history of Imath, we need to be certain
it's a good thing. Happy to discuss it further, but I'd suggest holding off
on this until after SIGGRAPH.
On Wed, Jun 26, 2019 at 1:44 PM Larry Gritz ***@***.***>
wrote:
> LGTM, but it's hard to judge whether this is the right configuration
> without seeing what the diffs look like when this is applied to the project.
>
> When I clang-format-ified OIIO, I went through a number of iterations of
> this config before it settled down, and only then did I make the big
> commits of all the source code post-reformatting.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#409>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFC3DGLAAXVRUJPN6XTU7CTP4PIKNANCNFSM4H3VPT3A>
> .
>
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
But in spite of that, I'm 100% in favor of committing the build targets and
the clang formating rules, just don't apply them to the source code yet.
…On Wed, Jun 26, 2019 at 2:00 PM Cary Phillips ***@***.***> wrote:
And about the space after the between the function name and parenthesis,
as in "foo (a, b, c)" that's Florian's formatting style, I believe he was
the origin of that convention. I've come to prefer it, but personal
preference aside, I'd tend to favor retaining it for that reason alone.
On Wed, Jun 26, 2019 at 1:57 PM Cary Phillips ***@***.***> wrote:
> Let's don't rush into this, I'd like to iterate on it until we're sure
> that the results are really worth it. I anticipate annoying a lot of people
> at ILM if we stomp on this revision history of Imath, we need to be certain
> it's a good thing. Happy to discuss it further, but I'd suggest holding off
> on this until after SIGGRAPH.
>
> On Wed, Jun 26, 2019 at 1:44 PM Larry Gritz ***@***.***>
> wrote:
>
>> LGTM, but it's hard to judge whether this is the right configuration
>> without seeing what the diffs look like when this is applied to the project.
>>
>> When I clang-format-ified OIIO, I went through a number of iterations of
>> this config before it settled down, and only then did I make the big
>> commits of all the source code post-reformatting.
>>
>> —
>> You are receiving this because you are subscribed to this thread.
>> Reply to this email directly, view it on GitHub
>> <#409>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AFC3DGLAAXVRUJPN6XTU7CTP4PIKNANCNFSM4H3VPT3A>
>> .
>>
>
>
> --
> Cary Phillips | R&D Supervisor | ILM | San Francisco
>
>
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
I once tried to figure out where the myFunc (int x) spacing convention came from as I felt Florian got it from somewhere else. I think it's from the old IRIS header files or something. I agree that the sooner we can start iterating on this format file the better. So, LGTM! |
The space between function and paren was also my preferred way to do it, as far back as I can remember. But I did it inconsistently, for example preferring |
Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>
I'm hoping the single largest change by bytes modified is actually the elimination of the mixed space and tab thing (replacing tabs with spaces, which means the code is indented consistently by all editors regardless of tab width setting). I realize this is an option to clang-format, but if we're going to do this, I'd like to make the code more readable by default regardless of editor config. There are a large number of extra spaces at the end of lines, which also get cleaned up... ;-P One thing which I can't decide how I feel is right now I have sorting of includes turned on (but not regrouping). Anyway, I've updated the rules a bit to match more the prevalent style and added a script to do a find and run clang-format so you can judge what happens. Hooray for git checkout... |
I jettisoned spaces before paren for a really simple reason. I have a habit of grepping for things that are called as functions, eg, search "foo(". With the space and no space if no parameter rule, the grep becomes complex, especially if foo() and foo(int bar) exist, because I have to grep "foo(" and also "foo (" to catch them all, or I have to write a regex to do something that would otherwise be "easy(". |
% grep "foo *("
…On Thu, Jun 27, 2019 at 10:01 AM Nick Porcino ***@***.***> wrote:
I jettisoned spaces before paren for a really simple reason. I have a
habit of grepping for things that are called as functions, eg, search
"foo(". With the space and no space if no parameter rule, the grep becomes
complex, especially if foo() and foo(int bar) exist, because I have to grep
"foo(" and also "foo (" to catch them all, or I have to write a regex to do
something that would otherwise be "easy(".
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#409>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFC3DGJ2FHYYHPSL5EZZWWLP4TW7ZANCNFSM4H3VPT3A>
.
--
Cary Phillips | R&D Supervisor | ILM | San Francisco
|
It's true that Windows based IDE's have supported regex for some years now. I just have to get used it ;) |
Signed-off-by: Kimball Thurston kdt3rd@gmail.com