[Development] White space / coding style patches welcome?

Axel Waggershauser awagger at gmail.com
Tue Mar 12 15:14:43 CET 2013

On Tue, Mar 12, 2013 at 2:11 PM, Oswald Buddenhagen
<oswald.buddenhagen at digia.com> wrote:
> On Tue, Mar 12, 2013 at 01:34:54PM +0100, Axel Waggershauser wrote:
>> 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
> the downside of doing it by type is producing a lot of history noise -
> commits you need to skip over when you are researching history.

Point taken.

> if the burden on the reviewer is too big, then you didn't split it up
> according to directories/modules sufficiently well.

What would be a sensible patch-size? Is there some reasonable maximum
number of lines/hunks?

> i did this exercise a few years ago already (just to have it discarded),
> and the outcome wasn't *that* huge (though there were some problem
> areas, like qtmultimedia and qt3support).

You mean, you prepared clean up patches and they were abandoned or
their effect vanished over time due to missing online checks?

> ok, let's modify the requirement: you don't need to set out to fix every
> problem. but in the lines you touch because of fixing specific problems,
> i want to see the remaining issues fixed as well.
> specifically, i want trailing whitespace and tabs/indentation fixed.
> the issue you started with is kinda optional for me.


> spaces around binary operators are kinda secondary, too. i don't even
> know how big the problem is.

Well, it was totally random but I liked that it was kind of a minor
issue resulting in a still smallish patch, perfect for a test balloon.
And that patch actually fixes all switch statements in qtbase.

Two thoughts/directions regarding support for automatic (sanity-check
like) tests:

1) 'simple' line-by-line checks running directly on the diff:

That should be rather easy to add as a more or less ad-hock
string-procssing check that looks for specific coding style problems
on a line-by-line basis in the diff, e.g. detecting classics like
"for( ... )" or even more obvious tabs and trailing whitespace issues.
Something like that could be integrated anytime without having a
complete conformant code base.

But it will easily reach it's limits like checking a
multi-line-statement and not having any clue whatsoever about the
context (complaints about random text in comments, maybe macro foo,

It will obviously be able to only address a subset of what is talked
about in http://qt-project.org/wiki/Qt_Coding_Style but maybe a
substantial portion and that without too much effort.

2) proper c++ parser based checks that require the patch to be applied
to the code to even run:

I don't know about the gerrit sanity checking infrastructure but can
imagine that would require a substantial change and it would also not
work without having a fixed source base first. But obviously be able
to do proper checks of any valid c++ code (I hereby presume there is
some fancy tool in the context of libclang available...).

I'd be inclined to look into option 1 rather than 2 as this seems to
be more manageable within a decent amount of time.

 - Axel

More information about the Development mailing list