git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Adam Spiers <git@adamspiers.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: git list <git@vger.kernel.org>
Subject: Re: [PATCH] Document conventions on static initialization and else cuddling
Date: Wed, 19 Sep 2012 22:14:03 +0100	[thread overview]
Message-ID: <20120919211403.GF19246@atlantic.linksys.moosehall> (raw)
In-Reply-To: <7vk3vpg2v1.fsf@alter.siamese.dyndns.org>

On Wed, Sep 19, 2012 at 01:43:46PM -0700, Junio C Hamano wrote:
> 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.

[snipped]

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

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

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

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

      reply	other threads:[~2012-09-19 21:14 UTC|newest]

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-02  0:12 [PATCH 0/9] new git check-ignore sub-command Adam Spiers
2012-09-02  0:12 ` [PATCH 1/9] Update directory listing API doc to match code Adam Spiers
2012-09-02  0:12 ` [PATCH 2/9] Improve documentation and comments regarding directory traversal API Adam Spiers
2012-09-02  0:12 ` [PATCH 3/9] Rename cryptic 'which' variable to more consistent name Adam Spiers
2012-09-02 19:56   ` Junio C Hamano
2012-09-02  0:12 ` [PATCH 4/9] Refactor excluded_from_list Adam Spiers
2012-09-04 12:32   ` Nguyen Thai Ngoc Duy
2012-09-02  0:12 ` [PATCH 5/9] Refactor excluded and path_excluded Adam Spiers
2012-09-04 12:40   ` Nguyen Thai Ngoc Duy
2012-09-04 17:23     ` Junio C Hamano
2012-09-05 10:28       ` Nguyen Thai Ngoc Duy
2012-09-06  3:21         ` Junio C Hamano
2012-09-06 12:13           ` Nguyen Thai Ngoc Duy
2012-09-06 14:59             ` Thiago Farina
2012-09-06 15:05               ` Nguyen Thai Ngoc Duy
2012-09-06 17:42                 ` Adam Spiers
2012-09-06 21:07                 ` Junio C Hamano
2012-09-02  0:12 ` [PATCH 6/9] For each exclude pattern, store information about where it came from Adam Spiers
2012-09-02 17:00   ` Philip Oakley
2012-09-02 19:02     ` Junio C Hamano
2012-09-02 22:36       ` Philip Oakley
2012-09-06 17:56         ` Adam Spiers
2012-09-02  0:12 ` [PATCH 7/9] Extract some useful pathspec handling code from builtin/add.c into a library Adam Spiers
2012-09-02  0:12 ` [PATCH 8/9] Provide free_directory() for reclaiming dir_struct memory Adam Spiers
2012-09-02  0:12 ` [PATCH 9/9] Add git-check-ignores Adam Spiers
2012-09-02 10:41   ` Nguyen Thai Ngoc Duy
2012-09-02 14:50     ` Adam Spiers
2012-09-02 20:38       ` Junio C Hamano
2012-09-03  4:14       ` Nguyen Thai Ngoc Duy
2012-09-02 20:41   ` Junio C Hamano
2012-09-03  1:47     ` Junio C Hamano
2012-09-20 19:46     ` [PATCH v2 00/14] new git check-ignore sub-command Adam Spiers
2012-09-20 19:46       ` [PATCH v2 01/14] Update directory listing API doc to match code Adam Spiers
2012-09-20 19:46       ` [PATCH v2 02/14] Improve documentation and comments regarding directory traversal API Adam Spiers
2012-09-20 19:46       ` [PATCH v2 03/14] Rename cryptic 'which' variable to more consistent name Adam Spiers
2012-09-20 19:46       ` [PATCH v2 04/14] Rename path_excluded() to is_path_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 05/14] Rename excluded_from_list() to is_excluded_from_list() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 06/14] Rename excluded() to is_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 07/14] Refactor is_excluded_from_list() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 08/14] Refactor is_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 09/14] Refactor is_path_excluded() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 10/14] For each exclude pattern, store information about where it came from Adam Spiers
2012-09-20 21:31         ` Junio C Hamano
2012-12-26 15:46           ` Adam Spiers
2012-09-20 19:46       ` [PATCH v2 11/14] Refactor treat_gitlinks() Adam Spiers
2012-09-20 19:46       ` [PATCH v2 12/14] Extract some useful pathspec handling code from builtin/add.c into a library Adam Spiers
2012-09-21  7:54         ` Michael Haggerty
2012-09-20 19:46       ` [PATCH v2 13/14] Provide free_directory() for reclaiming dir_struct memory Adam Spiers
2012-09-21  8:03         ` Michael Haggerty
2012-09-21 16:11           ` Junio C Hamano
2012-09-20 19:46       ` [PATCH v2 14/14] Add git-check-ignore sub-command Adam Spiers
2012-09-21  5:44         ` Johannes Sixt
2012-09-25 23:25           ` Junio C Hamano
2012-09-26  5:49             ` Johannes Sixt
2012-09-26 14:03               ` Junio C Hamano
2012-09-21  7:23         ` Michael Haggerty
2012-09-21 16:27           ` Junio C Hamano
2012-09-21 19:42         ` Junio C Hamano
2012-09-20 21:26       ` [PATCH v2 00/14] new git check-ignore sub-command Junio C Hamano
2012-09-20 21:43         ` Junio C Hamano
2012-09-20 23:45           ` Adam Spiers
2012-09-21  4:34             ` Junio C Hamano
2012-12-16 19:35               ` [PATCH 0/3] Help newbie git developers avoid obvious pitfalls Adam Spiers
2012-12-16 19:35                 ` [PATCH 1/3] SubmittingPatches: add convention of prefixing commit messages Adam Spiers
2012-12-16 23:15                   ` Junio C Hamano
2012-12-16 19:36                 ` [PATCH 2/3] Documentation: move support for old compilers to CodingGuidelines Adam Spiers
2012-12-16 19:36                 ` [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported Adam Spiers
2012-12-17  1:52                   ` Junio C Hamano
2012-12-17  2:15                     ` Adam Spiers
2012-12-17  4:18                       ` Junio C Hamano
2012-12-22 12:25                         ` Adam Spiers
2012-12-22 18:39                           ` Junio C Hamano
2012-12-26 15:44           ` [PATCH v2 00/14] new git check-ignore sub-command Adam Spiers
2012-09-21 19:00       ` Junio C Hamano
2012-12-16 23:04         ` compiler checks Adam Spiers
2012-09-24 22:31       ` [PATCH v2 00/14] new git check-ignore sub-command Junio C Hamano
2012-09-04 13:06   ` [PATCH 9/9] Add git-check-ignores Nguyen Thai Ngoc Duy
2012-09-04 17:26     ` Junio C Hamano
2012-09-05 10:25       ` Nguyen Thai Ngoc Duy
2012-09-10 11:15         ` Adam Spiers
2012-09-10 11:09     ` Adam Spiers
2012-09-10 12:25       ` Nguyen Thai Ngoc Duy
2012-09-10 16:30       ` Junio C Hamano
2012-09-02 20:35 ` [PATCH 0/9] new git check-ignore sub-command Junio C Hamano
2012-09-06 17:44   ` Adam Spiers
2012-09-07 10:03   ` Adam Spiers
2012-09-07 16:45     ` Junio C Hamano
2012-09-19 19:00       ` [PATCH] Document conventions on static initialization and else cuddling Adam Spiers
2012-09-19 20:43         ` Junio C Hamano
2012-09-19 21:14           ` Adam Spiers [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120919211403.GF19246@atlantic.linksys.moosehall \
    --to=git@adamspiers.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).