[Mesa-dev] [PATCH] i965/msaa: Only do multisample rasterization if GL_MULTISAMPLE enabled.

[Brian Paul]

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