[Development] White space / coding style patches welcome?

Axel Waggershauser awagger at gmail.com
Fri Mar 15 12:49:55 CET 2013


On Fri, Mar 15, 2013 at 8:11 AM, Thiago Macieira
<thiago.macieira at intel.com> wrote:
>> 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.

makes total sense...

>>     while (test == true
>>     ......&& 1 == 0) // 7 spaces == 0 relative to the '('
>
> Looks like Creator's special-case for ifs doesn't apply to whiles.

Indeed, and now that I think about it, this make sense, too, since the
"while (" is not 4 chars long, where as "if (" is.


>> 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:
>> [...]
>> 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.

It obviously did not. Here is the file:
examples/widgets/graphicsview/boxes/glextensions.cpp. So the macro is
defined in the same file and ends with a semicolon. My suggestion
would have been to 'move' the semicolon out of the macro definition,
so as to make it look like a standard function call (which seems
obviously better in my eyes). But in general, that might be a total
non-issue and only this one random example I happened to stumble
across.


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

I think so, too, but mixing them in one block is really plain 'horrid'
:-). Seen at least once. I agree with you that the first one looks
better. I guess even the proponents of the leading comma style would
not like this:

   char * array[] = {
      "some string"
      , "some other string"
   }

-> amend the official coding style wiki page?


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

Because there is code with leaning tabs looking like this:

     switch () {
     case 1:
<tab>   {
<tab>       some code here...
<tab>   }

and the default QtCreator's indentation for this is aligning the
braces with the 'c' of case, which makes it difficult to see where the
switch ends. Having that said, there is a setting in QtCreator (Blocks
relative to "case" or "default") that makes the whole block be
indented like above, but this is not the "Qt default style". Maybe the
current default style is just what is considered best practice by the
Qt community and I should not worry?

My point is, I'd rather not agree on a style where you have to
manually fight your editor's auto-indenting behavior.


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

? This is very example is all about whitespace and nothing else. My
understanding of 'whitespace' is 'stuff you don't see in your editor
and has no semantic impact on the c++ code', i.e. (certain) spaces,
tabs, new-lines. So style and whitespace issues are (mostly) the same
to me.

The whole point of this thread was to ask if this kind of patch is
welcome (before spending days on it only to have it rejected). So
'minor styling deviations' are acceptable for you. Now the question
is: what is minor? My idea was to propose fixes along the lines of the
'official' coding style: http://qt-project.org/wiki/Qt_Coding_Style.
So definitely stuff like fixing the following made up example (with
different types of ws problems):

-    if( a== f( (char*)x,y ))
-    {
+   if (a == f((char *)x, y)) {


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

Oh I see. So there is currently no hook mechanism in place that
allowed for a strict enforcement? Maybe the current sanitize-commit
mechanism is enough if people generally take the sanity bot warnings
serious? Or maybe it could be changed to report a '-2' sanity review
when really basic stuff (leading tabs, trailinng ws) is detected?


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

As explained above, that was about _moving_ the semicolon out of the
macro definition. Should have made that clear right from the start.


> I also recommend not touching the unnecessary braces. Those are usually the
> result of code changes inside the block.

OK.


> The non-whitespace churn is entirely unnecessary.

You are talking about the sanity bot warning or about some proposed
change of mine?

 - Axel



More information about the Development mailing list