On Thu, Feb 13, 2014 at 7:24 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
... Maybe, or maybe not. The basic issue still stands. New contributors will look at the existing code, and if its inconsistent how are they supposed to write code that's consistent in itself. That's not a question of pickiness but approach to the problem. ...
I'm not sure it's always clear what to do even the case of more experienced contributors, I've been taking a look at fixing a double unlock bug in RtlAcquireResourceExclusive ( http://source.winehq.org/source/dlls/ntdll/rtl.c#L157 ). And the style there indent-wise is so screwy that it's not clear what I should do in my patch. This one function currently has (level wise): 1) Four space indents 2) A tab indent + a space indent (in some cases just the tab) 3) A tab indent + a five space indent (alternatively, 13 spaces) 4) Two tabs + a space indent
So, what do I do? Do I just change line 190 to fix the bug and keep the two tab + a space indent? Do I fix just this line to conform to the standard since it's the only one I need to touch? Do I fix this one function because a lot of it doesn't conform to the standard? Personally, I would like to have a separate formatting patch to fix the function so that it's clear that I'm just fixing the one thing - but that's expressly prohibited under the current guidelines.
... My impression was that the answers to that were no and no, with the idea that the problem will be fixed by unifying the style together with other, functional changes. That way we end up with picky reviews and IMO that approach is bad. ...
I agree, Sebastian and I have been debating what to do with this function (and RtlAcquireResourceShared) for some time and style is the only thing keeping us from submitting the one-line fix for these. Honestly, it would be really great if we could run something like astyle on the whole codebase so that we were always working off of a "known good" style. If we could get that implemented then we could potentially think about verifying the patch style on submission and auto-rejecting patches that have style problems. Personally, I think that would be an amazing improvement to our submission process, but I have to use K&R style for work and I hate feeling like an idiot when I slip into the wrong style for a project.
Best, Erich