[Development] White space / coding style patches welcome?

[Axel Waggershauser]

With the trailing whitespace issue pretty much fixed, the next step is
eliminating leading tabs. A few questions about that:

1) If in doubt, is it generally fair to assume that whatever
QtCreator's CTRL+i in the default built-in Qt coding style does, is
the way to go?

2) Especially continuation lines are not really talked about in the
style guide wiki but are obviously a main and reoccurring issue when
replacing leading tabs. e.g. the QtCreator style suggest the following
(replaced ' ' with '.' for visibility):

    return GenFramebuffersEXT
    ........&& GenRenderbuffersEXT
    ........&& BindRenderbufferEXT;

So 8 space indenting, a bit much for my taste but I guess, someone
properly thought about that and this how new code looks like, too?

2 a) I found a place with 8 spaces indent but QtCreator would indent
it further to 12 so that the following 'Q' aligns with the 's'

    viewport()->setFlag(
    ............QGraphicsItem::ItemClipsChildrenToShape, true);

2 b) indenting a continuation of an assigment is only 8 spaces again:

    int hello =
    ........100 + test;

2 c) if vs. while lines seem obviously inconsistent

    if (test == true
    ........&& 1 == 0) // 8 spaces == 4 relative to the '('

    while (test == true
    ......&& 1 == 0) // 7 spaces == 0 relative to the '('

Bottom line: might it be the current auto-indent styling is actually
not completely trustworthy? (I hope the answer is 'no', otherwise this
would have to be 'fixed' first, I'd say. Comments?)

That is just stuff I ran across, I did not thoroughly checkout the
indentation behavior.

3) I came across a macro that had a semicolon at the end. I think this
is sub-optimal. Without special support in the IDE (like for the
Q_OBJECT macro) this leads to the following auto indented code:

....RESOLVE_GL_FUNC(GenFramebuffersEXT)
............RESOLVE_GL_FUNC(GenRenderbuffersEXT)

instead of

....RESOLVE_GL_FUNC(GenFramebuffersEXT);
....RESOLVE_GL_FUNC(GenRenderbuffersEXT);

Formally a non-whitespace change but in practice, it is.


4) What about consistent placement of the ',' in class member
initialization lists? I've seen

    : GvbWidget(parent),
      m_background()

and

    : GvbWidget(parent)
    , m_background()


5) About switch/case with blocks: I find the following indentation not
particularly easy to read

    switch (type) {
    case 1: {
        int a;
        return a;
    }
    default:
        return 0;
    }

probably not having a majority appeal but I think the following
(QtCreator auto-indent-compatible) version would be better:

    switch (type) {
    case 1: {
        int a;
        return a; }
    default:
        return 0;
    }


Tooling Question: is there some command line tool that can execute the
editors CTRL-i functionality? If not, is that code deeply interwoven
with the editor gui or might I be able to quickly hack such a tool?

>> I see. Regarding false report rate, I think that checking for trailing
>> ws as well as checking for leading tabs should be secure enough to
>> actually strictly enforce them. See also 'git diff --check'.
>>
> yes. there are a few legitimate exceptions, but most of the files are so
> seldomly touched that it's ok to deal with them when it happens.

Please note that the above comment ('secure enough') was made with
only c++ source files in mind. I can't say anything about extending
that enforcement to other text based files (txt, rc, pro, conf, xml,
pl, qdoc, etc...).

Where are those existing enforced checks implemented (like the check,
whether you have a valid Change-Id in your commit message)?


>> But I think the whole "mixed commit' issue will become more or less
>> irrelevant, if all issues in the current source base are solved, as
>> there would be simply no 'collateral' ws-fixes in the future.
>>
> hopefully. i introduced that check as a direct response to the rejection
> of my previous attempt at a global cleanup, so it would be logical if it
> became obsolete now.

So you are _the_ Guy to talk to about whitespace in Qt ;-). Regarding
the above mentioned problem with missing semicolons, I might suggest
to accept some patches that are indeed non-white-space only. The same
would be the case with stripping braces from one-line if|for|while
blocks.

 - Axel