the inevitable C formatting/brace style flamewar

[Michael Stahl]

On 16/08/12 18:22, Stephan Bergmann wrote:
> On 08/16/2012 06:13 PM, Michael Meeks wrote:
>> 	Looks sensible :-) pushed to -3-6 (though I personally loathe burning
>> two lines of non-renewable vspace for no good reason ;-).

you are _half_ right: burning 2 lines for that is silly :)

> The "{" and "}" lines you mean?  I /do/ consider them helpful; spent a 
> long time scratching my head just the other day again trying to grok 
> what's going on---when all that was going on was a deceivingly formatted 
> single-statement if-condition to which another statement had been added 
> (not).
> 
> Incidentally, noticed a couple days ago a commit of yours where you 
> removed such "{" ... "}" lines, in a mumbo rebase commit, so thought it 
> was likely more a rebasing artifact than deliberate doing.  Anyway, I 
> would appreciate it if such lines were /not/ removed.

but i certainly agree with Stephan that adding { } to every if, for etc.
is a Good Thing.

if memory serves i've reviewed 2 patches in the last 3 months or so that
added a statement to a 1-line then or else clause that was indented but
did not have braces, and the author indented his new line and forgot the
braces.  clearly that indicates to me that syntax significant
indentation (as in Haskell or Python) is an excellent idea that prevents
bugs, and in a language that lacks this convenience always adding the
braces is the next best you can do.

but the braces only burn 2 lines of vspace instead of 1 because of our
silly coding convention, with which i disagree strongly in this aspect.
 apparently somebody thought it a great idea to mandate Allman style for
large parts of OOo, which is a) ugly and b) unnecessarily wastes 1 line
of vspace on very short blocks.

what i would like instead is a context-sensitive and common-sense based
coding style:

1) if the condition is long, then put the opening brace on a new line.

rationale: in this case it's good to visually separate the condition
from the compound statement, e.g. this is a PITA to read:

>                             if ( !pFly->Lower() || !pFly->Lower()->IsNoTxtFrm() ||
>                                  !((SwNoTxtFrm*)pFly->Lower())->HasAnimation())
>                                 pFly->RefreshLaySubsidiary( pPage, rRect );

>         if (bCalledFromShell && !lcl_IsItemSet(*pNewTxtNd, RES_PARATR_ADJUST) &&
>             SFX_ITEM_SET == pAnchorNode->GetSwAttrSet().
>             GetItemState(RES_PARATR_ADJUST, sal_True, &pItem))
>             static_cast<SwCntntNode *>(pNewTxtNd)->SetAttr(*pItem);

2) if the condition is short, then put the opening brace on the same line

rationale: in this case it's immediately obvious where the condition
ends, so no need for an extra vspace wasting line

so this:

>     if( pCT->pInsBox )
>     {
>         pCT->pInsBox->GetTabLines().push_back( pNewLine );
>     }
>     else
>     {
>         pCT->pTblNd->GetTable().GetTabLines().push_back( pNewLine );
>     }

could be much shorter, with only 20% brace line overhead:

>     if( pCT->pInsBox ) {
>         pCT->pInsBox->GetTabLines().push_back( pNewLine );
>     } else {
>         pCT->pTblNd->GetTable().GetTabLines().push_back( pNewLine );
>     }