[Development] White space / coding style patches welcome?

Thiago Macieira thiago.macieira at intel.com
Fri Mar 15 16:12:32 CET 2013


On sexta-feira, 15 de março de 2013 12.49.55, Axel Waggershauser wrote:
> > Put the semi-colon if the semi-colon is necessary. Otherwise, don't. This
> > is not optional.

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

And that is fine. I do recommend moving the semi-colon from inside the macro to 
outside it, especially so that they look like regular C statements.

But that's not a whitespace change. It's an actual code change. You're welcome 
to do it, just remember to write properly in the commit message :-)

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

That's one of the issues I have with the default Qt style. It allows for two 
braces closing on exactly the same level (think of the last case or default). 
The Qt style allows and even asks for it, so keep it as it is.

Recently Creator has been suggesting a variant, like the special-casing of the 
ifs, that make it visually more pleasant. You're welcome to use them in new 
code and in fixing tabs for old code, but please don't modify existing already-
compliant cases to match.

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

Unfortunately newlines are very different. Git blame -w and other whitespace-
ignoring options in other programs work on a line-by-line basis. They will not 
catch the moving of braces to the previous or next line. That's what I call 
unnecessary churn.

So do not change the non-whitespace content of the line unless you really have 
to (like fixing code that is completely out-of-style).

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

Here's what I think:

- adding the space between if and ( is fine and a very welcome change
- fixing the rest of the whitespace in that line is fine if you're making it 
more visually pleasant
   - exception: when spaces were intentional, in long and complex nesting 
chains (there are a few in QtDBus)
- moving the brace should be avoided, except as noted above

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

Like I said, that's fine, but it's not a whitespace change. That also means you 
cannot do it in the same commit as the rest.

-- 
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/338e2a48/attachment.sig>


More information about the Development mailing list