[PATCH] Add utilities to check style

[Martin Schwenke]

Hi Ralph,

On Fri, 4 May 2018 09:11:49 +0200, Ralph Böhme <slow at samba.org> wrote:

> On Fri, May 04, 2018 at 02:02:07PM +1000, Martin Schwenke via samba-technical wrote:
> > On Thu, 26 Apr 2018 15:31:32 -0400, Simo via samba-technical
> > <samba-technical at lists.samba.org> wrote:
> >   
> > > On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical
> > > wrote:  
>  [...]  
>  [...]  
>  [...]  
> > > 
> > > Ok, I am going to strip any checking for parens then
> > > because both == no stlye == no check we can enforce.
> > > 
> > > Attached find a patch with the check disabled (but the code is changed
> > > to reflect my last proposal, so if we change mind at least we have the
> > > code ready to be used).  
> > 
> > I think this is good, useful and very close, so we should get it into
> > the tree because it is useful.  So:
> > 
> > Reviewed-by: Martin Schwenke <martin at meltin.net>  
> 
> please don't push before the issue I ran into is sorted out. I couldn't really
> test and review the patch due to that.

Darn.  Sorry, I forgot that I'd seen that and got too keen.  :-(

> > It still see minor issues and will argue against including this in
> > tests until it more closely reflects README.Coding.
> > 
> > Although it doesn't produce a warning or exit non-zero, the indent
> > output makes this recommendation:
> > 
> >  	ctdb->notification_script = talloc_asprintf(ctdb,
> > -						    "%s/notify.sh",
> > -						    ctdb_base);
> > +						    "%s/notify.sh", ctdb_base);
> > 
> > This contradicts the change to README.Coding made in commit
> > 614f5a041ea6e021c2ff9c4e462b0e22626c7f33.  I don't think there's a way
> > of telling indent to leave such lines alone.  :-(  Perhaps we should
> > avoid printing the indent output?  
> 
> Hm. Imho the script must support our recommended way of indenting
> functions and macros. If it doesn't support that we can't really use
> it.

True, but if it helps to find useful issues then it is better than
nothing.

At the moment both warnings and the final diff is printed to stdout.
One possibility could be to make the diff optional, so that we can just
concentrate on the warnings.  Alternatively, we could send the warning
to stderr and the diff to stdout... and potentially ignore the
diff.  :-)

> > Speaking of that change to README.Coding, I would like to see it
> > changed to a recommendation rather than a a requirement.  Making
> > code take up needless vertical space makes it harder to read the
> > code around it.  
> 
> The idea of the rules in README.Coding is to result in better
> readable code. As every rule can easily degrade when applied without
> thinking, imho the rules should be recommendations and not
> requirements anyway.

I think some of the simple, well-defined things (e.g. untidy whitespace)
can always be requirements.  If we're going to do any automatic checking
when we need to distinguish between requirements and recommendations...

> > If you look at a logging call that includes several short variables
> > but where the format string is enough long to cause a line break
> > then you can end up with something that is horrible to read.
> > Here's a made-up example:
> > 
> >         DBG_INFO("Failed in iteration %u for lmaster=%u, dmaster=%u
> > (%s)\n", i,
> > 		 state->lmaster,
> > 		 state->dmaster,
> > 		 strerror(ret));  
> 
> Iirc README.Coding doesn't mention macros, so we could keep the rule
> for function calls but relax it for macros. I fully agree that those
> multine debug macros are a nuisance and a perfect example how a sane
> rule degrades it something not so sane. :)

Yeah, I wondered about that... :-)

...  but then you get to printf()/fprintf() and you find yourself on a
slippery slope!

> > This could easily fit on 2 lines instead of dominating the code
> > around it.
> > 
> > I think that for function calls we're better off letting people use
> > their discretion.  
> 
> The idea was to minimize diffs of future changes that add or remove
> function parameters. The one-arg-per-line format produces well
> readable code and applying the rule is simple and doesn't require
> reshuffling to adhere to the 80 lines reule while WIP code evolves.
> That's the no 1 reason why I like it. :)

Well, we need to balance overall readability of code against minimising
diffs.

I like it in a lot of cases too.  However, I think that there are cases
where a simple function call could easily grow to 6 or 7 lines and
dominate more important code around it.  If unnecessary vertical space
in a couple of function calls stops me from being able to fit a function
on my screen, that makes the code harder to read and understanding, and
it is having a negative overall effect.

... and the bike shed definitely has to be purple!  ;-)

peace & happiness,
martin