[Development] White space / coding style patches welcome?

Thiago Macieira thiago.macieira at intel.com
Fri Mar 15 08:11:00 CET 2013


On sexta-feira, 15 de março de 2013 04.16.46, Axel Waggershauser wrote:
> 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?

Right. Emacs in indent-tabs=nil mode is also quite acceptable.

> 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?

Line up under the parenthesis nesting level if there is one, simple 
indentation (4 or 8 spaces) if there isn't one like above.

These corner-cases are usually solved by "best visual elegance".

> 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);

I'd have indented to inside the (...

> 2 b) indenting a continuation of an assigment is only 8 spaces again:
> 
>     int hello =
>     ........100 + test;

Right.

> 
> 2 c) if vs. while lines seem obviously inconsistent
> 
>     if (test == true
>     ........&& 1 == 0) // 8 spaces == 4 relative to the '('

This is a special case because of a {

If you write:
    if (test == true
        && 1 = 0) {
        hello();
    }

Then "hello" and && line up, making it visually ambiguous. The extra indent is 
to prevent that.

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

Looks like Creator's special-case for ifs doesn't apply to whiles.

> 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.

As I said, do what makes it look best. It's subjective.

> 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.

Put the semi-colon if the semi-colon is necessary. Otherwise, don't. This is 
not optional.

The IDE should expand the macro and realise that a semi-colon is not 
necessary, thus not concluding it's a continuation.

Suggestion: don't touch the semi-colons. Just fix the leading tabs.

> 4) What about consistent placement of the ',' in class member
> initialization lists? I've seen
> 
>     : GvbWidget(parent),
> 
>       m_background()
> 
> and
> 
>     : GvbWidget(parent)
> 
>     , m_background()

I prefer the former, though I've seen people do the latter because it allows 
for adding new members without touching a previous line. I find lines starting 
with a comma horrid.

Both are acceptable.

> 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;
>     }

Why are you asking this?

Don't touch the styling. Just fix the whitespaces and tabs. I will reject any 
patch that tries to fix minor styling deviations in QtDBus and QtCore. Do it 
only if the code is completely out-of-style or if it's about whitespace.

The Qt coding style guide says that you're allowed to deviate if it makes the 
code better.

> >> 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...).

qmake .pro files, perl scripts, and our XML files are tab-free in Qt. Obviously, 
you should not touch the test data or 3rd-party code.

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

The sanity bot runs on all pushes to Gerrit. That has nothing to do with the 
Change-Id check, which is implemented by Gerrit itself.

> 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.

Do not add or remove semi-colons. If the semi-colon is present and shouldn't 
be there, it should be fixed as a code fix, not as a style change. If there is 
no semi-colon, do not add one.

I also recommend not touching the unnecessary braces. Those are usually the 
result of code changes inside the block. The non-whitespace churn is entirely 
unnecessary.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/development/attachments/20130315/9a0b1381/attachment.sig>


More information about the Development mailing list