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.
In the meantime, I'd like to keep things at least somewhat consistent
within files. But I did give you my Reviewed-by, so you're free to push
it---I'll just be somewhat frustrated, that's all.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
