[PATCH] Add utilities to check style

[Ralph Böhme]

Hi Martin,

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

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.

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

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

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

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

-slow

-- 
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG Key Fingerprint:           FAE2 C608 8A24 2520 51C5
                               59E4 AA1E 9B71 2639 9E46