Re: [PATCH] Document conventions on static initialization and else cuddling

> Adam Spiers <git@adamspiers.org> writes:
> 
> > Signed-off-by: Adam Spiers <git@adamspiers.org>
> > ---
> >
> > I have begun work on fixing existing code to adhere to these
> > guidelines on braces, but there are currently a lot of violations,
> > which means any patches to fix them would be large.  So before I spend
> > any more time on it, I would like to check whether such patches would
> > be welcomed? And if so, should I be doing that on the master branch?
> 
> In general, no.

> > I have also added some simple rules such as `check-brace-before-else'
> > to the top-level Makefile which perform appropriate `grep -n' commands
> > to detect violations and for example easily fix them via emacs' M-x
> > grep.  These rules can be invoked together via a `check-style' target.
> > Would this also be welcomed?  If so, should the checks all be
> > introduced in a single commit, or each check along with the code which
> > was fixed with its help?
> 
> I am not enthused, personally.

> >  Documentation/CodingGuidelines | 42 +++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> > index 57da6aa..1a2851d 100644
> > --- a/Documentation/CodingGuidelines
> > +++ b/Documentation/CodingGuidelines
> > @@ -117,17 +117,49 @@ For C programs:
> >     "char * string".  This makes it easier to understand code
> >     like "char *string, c;".
> >  
> > + - We avoid unnecessary explicit initialization of BSS-allocated vars
> > +   (static and globals) to zero or NULL:
> > +
> > +	static int n;
> > +	static char **ch;
> > +
> > +   rather than:
> > +
> > +	static int n = 0;
> > +	static char **ch = NULL;
> 
> As I said, I am in general not in favor of piling more rules here,
> but we've seen this one in new contribution often enough that it is
> a good addition.

> > +
> > +   These are superfluous according to ISO/IEC 9899:1999 (a.k.a. C99);
> > +   see item 10 in section 6.7.8 ("Initialization") of WG14 N1256 for
> > +   the exact text:
> > +
> > +     http://open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
> 
> But I do not think this explanation is necessary.  "This is one of
> our house rules" is a sufficient reason people, who want to blindly
> obey guidelines, need to see.

> >   - We avoid using braces unnecessarily.  I.e.
> >  
> >  	if (bla) {
> >  		x = 1;
> >  	}
> >  
> > -   is frowned upon.  A gray area is when the statement extends
> > -   over a few lines, and/or you have a lengthy comment atop of
> > -   it.  Also, like in the Linux kernel, if there is a long list
> > -   of "else if" statements, it can make sense to add braces to
> > -   single line blocks.
> > +   is frowned upon.  A gray area is when the statement extends
> > +   over a few lines, and/or you have a lengthy comment atop of
> > +   it.  Also, like in the Linux kernel, it can make sense to add
> > +   braces to single line blocks if there are already braces in
> > +   another branch of the same conditional, and/or there is long
> > +   list of "else if" statements.
> 
> Reflowing text without reason is frowned upon as it makes the
> review unnecessary harder by hiding where things have changed.

> You are suggesting to
> 
>         /* Turn this into ... */        /* ... this instead */
>         if (condition)                  if (condition) {
>                 true;                           true;
>         else {                          } else {
>                 otherwise;                      otherwise;
>                 ...                             ...
>         }                               }
> 
> I do not think such an update is wrong per-se, but among different
> shades of gray, the left is probably one of the most minor kind of
> violations.