On 06/19/2012 06:18 PM, Kenneth Graunke wrote:
On 06/19/2012 11:27 AM, Paul Berry wrote:
On 19 June 2012 10:29, Kenneth Graunke wrote:
Whitespace errors here. I know Mesa isn't terribly consistent
w.r.t. tabs or spaces, but in general these files use 3 space indent
with 8 space tabs. Please use tabs to match the surrounding lines.
I'm reluctant to add fuel to coding convention debates, but I really
don't think "follow whatever tab/space style the surrounding code
follows" is a reasonable coding convention to enforce. Most editors do
automatic indentation of C/C++ code, and can be easily configured to
either globally use tabs or globally not use tabs. They can't be easily
configured to follow the tab/space style of the surrounding code, which
means that if this is our coding convention, it's going to be an extra
developer burden.
Also, our documentation says we prefer spaces to tabs. From
docs/devinfo.html:
Here's the GNU indent command which will best approximate my
preferred style: (Note that it won't format switch statements
in the preferred way)
indent -br -i3 -npcs --no-tabs infile.c -o outfile.c
That "--no-tabs" option causes indent to use spaces, not tabs.
And the toplevel .emacs-dirvars file (which emacs automatically
consults
if the dirvars package is in use) configures emacs to automatically
indent using spaces, not tabs, using this line:
indent-tabs-mode: nil
When I started working on Mesa code over a year ago I configured my
editor to insert spaces rather than tabs (on Chad's recommendation),
and
the only time people have found fault with my whitespace has been
situations like this, where I was editing code that didn't follow our
documented convention. So I really think there is a good precedent for
spaces being our preferred indentation style, in spite of the fact that
Mesa code is very inconsistent about following it.
My preference would be to leave the patch as is. If that's not
acceptable, I'd propose changing tabs to spaces in the parts of the
code
that I'm modifying, so that at least this patch moves us toward
consistent use of spaces instead of tabs, rather than away from it.
I'm sorry for being petty, but I simply don't understand why you need
to make this into a large debate every time. You already spent more
time writing a lengthy email response about why you don't need to
follow the existing style than it would take to incorporate my feedback.
As I understand it, the rule is: be consistent with the existing code,
or (if it's totally wrong or internally inconsistent) make a wholesale
change to fix it, independent of actual content changes. I hold
everyone to this standard, including new developers and people
contributing in their free time. Projects like the Linux kernel reject
patches outright when they don't follow indendtation style. Some
people may think that's silly, but they deal with it anyway.
In this particular case, when you add the _NEW_MULTISAMPLE dirty bit,
your patch adds inconsistency within a single assignment statement:
.field = _NEW_FOO |
<tabs> _NEW_BAR |
<spaces>_NEW_MULTISAMPLE
That's what I really find offensive. If we can't even be consistent
within a single line of code...sheesh. Please be consistent and use tabs.
That said, I agree with you that it would save time in the long run to
simply pick one and use it consistently. Apparently, there was a
misunderstanding at one point - Brian's coding style has always been
three space indent, no tabs, but someone mistakenly began using them.
Now, the whole GLSL compiler and most of our new code is mixed tabs
and spaces.
At some point, I'd like to pick one, get everyone to agree, and
mass-convert everything in one fell swoop. Also, distribute the
appropriate emacs and vim settings to indent things correctly. Then we
can move forward. My preference is to go back to the documented 3
space, no tabs style---it's much simpler to explain, and much harder
to screw up than mixing the two. It sounds like that's your preference
as well. We just need to agree on it and do it.
Just to chime in, I'd (of course) like everyone to follow the 3-space,
no tabs indentation style. (BTW, years ago I recall someone
justifying 3 because "2 isn't enough, 4 is too much". I liked that.)
However, I haven't been super strict about it. In the case of drivers
where I have no stake I'm fine with letting those developers use
whatever style suites them. So some of the radeon drivers, for
example, use 8-space tab indentation. But I've been more picky about
style in submitted patches to maintain consistency.
As you've found out though, the real frustration is when there's a mix
of indentation styles in one file. In that case I sometimes reformat
old code. Basically, do whatever seems prudent for the situation at
hand. But do the reformatting after the bug fix so that cherry
picking to the stable branch doesn't require picking the formatting
change first.
If people want to re-indent whole files I'm OK with that, but I don't
feel too strongly about it.
BTW, I've seen inconsistent formatting in other projects too (Linux
kernel, LLVM, etc) so I'm not going to get too worked up over it.
-Brian
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
