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

Adam Spiers [off-list ref] writes:

Signed-off-by: Adam Spiers <redacted>
---

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.

The cost/benefit ratio of carrying such a change is very high.

The guidelines are not a set of hard rules that any and all code in
violation have to be fixed right away.  I am reluctant to pile more
detailed "rules" in the existing list in the document to give it an
incorrect impression that they are hard rules that all lines should
abide by, which inevitably leads to people who end up wasting time
on creating "style fixes" patches whose only practical effect is to
collide with other topics in flight.

Especially if such a "style fix" is done as a single patch, it will
block all the other topics that happen to touch the overlapping
regions for reasons more important than "style fix".

To mitigate the risk of such unnecessary collisions, while improving
the health of the codebase, we tend to do two things:

 - review new code and make sure it does not introduce guideline
   violations.  Two sets of new code that touch overlapping area by
   definition have to collide and their conflicts resolved anyway,
   so this does not add any additional cost to the project.

 - before updating existing code, clean up the area and small
   surrounding area it touches as a preparation step of the patch
   series.  As long as "surrounding area" is kept sufficiently small
   (the preparation step may have to refrain from touching the whole
   file, depending on what other topics are still in flight), this
   again does not add any additional cost to the project.

There is a third-option that we tend try to avoid but occasionally
do take:

 - update a dormant code that nobody has touched for a long time and
   no discussion on the list (with or without patch) is likely to
   result in a new topic that might want to touch it.

By the way, I have a handful of my favorite styles that are not
listed in the document, and when I write new code myself I try to
stick to them, but I do not raise a red (or any color) flag when
seeing "violating" code in a new patch [*1*].

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.

BTW I briefly tried to find an existing tool out there which could
already do the checking for us, but only found ones like uncrustify
which rewrite code rather than warning on inconsistencies.  I also saw
that the kernel's scripts/checkpatch.pl only worked with patches and
was also extremely kernel-specific in the nature of its checks.

The checkpatch.pl script works with patches for a very good
reason. It is a tool to look for "new" problems patches
introduce.

I often run "checkpatch.pl --no-tree" on incoming patches, by the
way.

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.



[Footnote]

*1* An example of them is "when 'if (...) ... else ...' chooses
between two equally plausible conditions, write it in such a way
that the body of the else clause runs longer".  That is because
it is far easier to get confused about the condition when looking at
what the body in the "else" clause is doing after a long body in the
"if" clause:

        /* BAD */                       /* GOOD */
        if (condition) {                if (!condition) {
            ....                                ....
            ....                                ....
            ....                        } else {
            ....                                ....
            ....                                ....
            ....                                ....
            ....                                ....
        } else {                                ....
            ....                                ....
            ....                                ....
        }                               }

But that can safely be done only when "condition" and "!condition"
are both equally natural expressions.

On Wed, Sep 19, 2012 at 01:43:46PM -0700, Junio C Hamano wrote:

Adam Spiers [off-list ref] writes:

quoted

Signed-off-by: Adam Spiers <redacted>
---

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.

[snipped]

quoted

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.

OK, I'll drop work on these then.

[snipped]

quoted

 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.

Yes, I am proposing its addition precisely because its current
omission resulted in a small but non-negligible amount of both my time
and yours being wasted in a recent patch review cycle.

quoted

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

I presume I'm not the only person who wishes to obey guidelines but
also understand why they exist.  In general, a deeper understanding of
the purpose behind guidelines helps improve judgement about when and
how to apply them.

quoted

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

There was a reason, it just wasn't a particularly good one :-) (emacs
reflows by paragraph, so you have to go out of your way to stop it
reflowing existing text.)

BTW, for anyone else reading who is confused by the fact that the
quoted hunk above shows no signs of reflowing, I think the hunk itself
has been re-reflowed back to the original width (presumably
deliberately in order to determine what really 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.

I was under the impression it was _your_ suggestion ;-)

  http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204975

Perhaps I read too much into that, since it was concerning the mirror
image case (i.e. where the 'else' branch is a single statement and the
'if' branch requires braces), but they seem like stylistically
equivalent debates to me.

Thanks for the review, Junio.  Based on my responses above, what would
you like me to do with the patch (if anything)?