[Development] White space / coding style patches welcome?

Axel Waggershauser awagger at gmail.com
Tue Mar 12 13:34:54 CET 2013


>> 3. How to best split up patches from that category? e.g. fixing all
>> "if(...){" -> "if (...) {" occurrences in qtbase would be a rather
>> large patch (I have it half done)
>>   a) split that up based on "manageable" subdirectories?
>>
> yes, and that only for easy recovery from merge conflicts (which you
> will get quite some of in highly active areas). other than that, make it
> one huge whitespace cleanup change: remove trailing whitespace, expand
> tabs (which for fun have inconsistent expectations regarding their
> size), fix indentation, fix mixplaced line breaks (the brace re-wrapping
> falls into this category), add missing spaces, etc.

I thought fixing everything at once everywhere is obviously a bad idea:
 * merge conflicts until the end of time
 * too much burden to come up with and also to review the patch

Especially the combination is problematic: a huge patch will only be
looked at when someone is 'really bored', because it takes such an
effort, the longer the patch lays around, the more likely a merge
conflict.

Hence, splitting it up is inevitable. I thought of two split up strategies:
 a) split up by the 'type' of style problem (like the above mentioned change)
 b) split up by file/directory and try to fix every 'type' at once

I thought splitting by type first and potentially by directory second
if it is too big still, would be best because it reduces the burden on
the reviewer: only one type of change should be easier to skim over
and might actually be checked by some ad-hock script if that was
necessary. Or does the automatic check for white-space only changes
make it secure enough so it can basically be waved through without
having to look at each hunk?

> somebody attempted this just a few days ago, but gave up when the scope
> of the undertaking became clear. i can't find the change now ...

I don't think the requirement to only accept a patch if it fixes
'every' whitespace problem in one go would be reasonable as it would
result in exactly this situation: too much work to do/review it all at
once so one gives up.

Therefore I propose to accept fixes, even if they don't fix every
issue (either in type of file space) at once: better improve something
step by step, than nothing at all.

>>   b) only do it in files where it is used inconsistently / where the
>> wrong occurrences are the minority?
>>
> no, do it consistently - to have a good example everywhere, and no
> excuses for messing up. then we can make the automatic checking
> stricter, too (which lars said was a precondition for even bothering
> with a cleanup to start with).

As a final goal, it should obviously be consistent everywhere but I
thought it makes sense to make a difference between files where there
are some random style issues here and there (most of the code) and
files that are written pretty consistently but according to a
different style guide (like the qmake generator stuff). The latter
might probably be best fixed in an 'all types at once but on a
file/directory basis' approach, so each patch would increase the
'local' consistency and be an improvement even if I or someone else
would loose interest at some point in their white space cleaning
spree.

To summarize my suggestion:
 * accept partly style fixes (in either type or file space)
 * accept them on an a timely manner to make it feasible at all with
regards to merge conflicts
 * intentionally ignore stuff in src/3rdparty

I might even be interested in looking into the automatic style check
issue, but can't promise anything.

> have fun ^^

Thanks ;) Depending on your feedback, I'll give it a try. ;)

 - Axel



More information about the Development mailing list