[PATCH] Add utilities to check style

[Martin Schwenke]

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:
> > On Thu, Apr 26, 2018 at 02:49:02PM -0400, Simo wrote:  
> > > 
> > > As I said README.Config contradicts itself, it says one thing but
> > > points to an indent tool that does another.
> > > 
> > > So far You raised concerns about clarifying this rule in favor of what
> > > the indent tool does, while Jeremy supported the opposite.
> > > 
> > > I am not married to either really, but I also prefer the { always on
> > > the same line. I think we need to decide which way to go.
> > > 
> > > Anyone else has opinions ?  
> > 
> > You know what they say about opinions :-). Yeah, I guess
> > this is just a style thing - you and I like it one way,
> > Ralph likes another. In those cases we should probably
> > allow both IMHO.  
> 
> 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>

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?

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.

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

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.

Sorry I didn't get a chance to complain about this at the time that
change was proposed.  :-(

peace & happiness,
martin