[PATCH] Add utilities to check style

[Simo]

On Wed, 2018-04-25 at 21:26 -0400, Simo via samba-technical wrote:
> On Wed, 2018-04-25 at 07:58 +0200, Ralph Böhme via samba-technical
> wrote:
> > On Tue, Apr 24, 2018 at 02:03:06PM -0700, Jeremy Allison via samba-technical wrote:
> > > On Tue, Apr 24, 2018 at 04:56:59PM -0400, Simo via samba-technical wrote:
> > > > Attached a new patch with some of the requested fixes.
> > > > 
> > > > However while reading README.Coding I noticed we suggest a style I do
> > > > not fully agree with (and that is also flagged by the current style
> > > > checker).
> > > > 
> > > > we say that something like:
> > > > 
> > > > 	for (longline;
> > > > 	     longline;
> > > > 	     longline)
> > > > 	{
> > > > 
> > > > should be used instead of always keeping the opening on the same lne as
> > > > the closing ')' like:
> > > > 
> > > > 	for (longline;
> > > > 	     longline;
> > > > 	     longline) {
> > > > 
> > > > 
> > > > I strongly prefer this second form for consistency, however if people
> > > > feel the first form is ok I will simply drop all braces checks as we
> > > > have no way to easily check for brace conformance anymore.
> > > > 
> > > > Note that the indent script we suggest to use in README.Coding will
> > > > also generate the second form, so we are, at best, inconsistent :-)
> > > 
> > > +1 from me on the second-form only. I much prefer the brace on the
> > > opening line rather than the line below.
> > 
> > -1. The same applies to multiline if statements where we require the first form
> > for quite some time.
> > 
> > The example in README.Coding doesn't give a good example of why the second form
> > gets ugly for the case the "longline" is really a long line.
> > 
> > Example from source3/winbindd/winbindd_pam.c:
> > 
> >         if (NT_STATUS_IS_OK(result)
> >             && (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
> >             && lp_winbind_offline_logon()) {
> >                 result = winbindd_update_creds_by_name(contact_domain, user,
> >                                                        newpass);
> > 
> > Putting the opening brace on a seperate line makes it easier to spot the if
> > clause condition vs the if body:
> > 
> >         if (NT_STATUS_IS_OK(result)
> >             && (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
> >             && lp_winbind_offline_logon())
> >         {
> >                 result = winbindd_update_creds_by_name(contact_domain, user,
> >                                                        newpass);
> > 
> > Example from source3/smbd/smb2_create.c that gets it right:
> > 
> >         if (((state->dhnc != NULL) && (state->dh2c != NULL)) ||
> >             ((state->dhnc != NULL) && (state->dh2q != NULL)) ||
> >             ((state->dh2c != NULL) && (state->dhnq != NULL)) ||
> >             ((state->dh2q != NULL) && (state->dh2c != NULL)))
> >         {
> >                 /* not both are allowed at the same time */
> >                 return NT_STATUS_INVALID_PARAMETER;
> >         }
> > 
> > Written with the opening brace on the same line:
> > 
> >         if (((state->dhnc != NULL) && (state->dh2c != NULL)) ||
> >             ((state->dhnc != NULL) && (state->dh2q != NULL)) ||
> >             ((state->dh2c != NULL) && (state->dhnq != NULL)) ||
> >             ((state->dh2q != NULL) && (state->dh2c != NULL))) {
> >                 /* not both are allowed at the same time */
> >                 return NT_STATUS_INVALID_PARAMETER;
> >         }
> > 
> > In this form your eyes start scanning for the brace to seperate the if condition
> > from the body while when written correctly you can immediately seperate both.
> 
> The easy way is to use a blank line:
> 
> 	if (((state->dhnc != NULL) && (state->dh2c != NULL)) ||
> 	    ((state->dhnc != NULL) && (state->dh2q != NULL)) ||
>             ((state->dh2c != NULL) && (state->dhnq != NULL)) ||
>              ((state->dh2q != NULL) && (state->dh2c != NULL))) {
> 
>                  /* not both are allowed at the same time */
>                  return NT_STATUS_INVALID_PARAMETER;
>          }
> 
> IMHO a better way to deal with such a long statement is to use a
> MACRO instead.
> 
> #define WHATEVER_STATE_NOT_NULL(s) \
> 	(s->dhnc || s->dh2c || s->dhnc || s->dh2q || \
> 	 s->dh2c || s->dhnq || s->dh2q || s->dh2c)

Clearly not same semantics, but it doesn't change the point :-)

Corrected:
#define NOT_BOTH_AT_THE_SAME_TIME(s) \
	((s->dhnc && s->dh2c) || \
	 (s->dhnc && s->dh2q) || \
	 (s->dh2c && s->dhnq) || \
	 (s->dh2q && s->dh2c))

> then that if turns into:
> 
> 	if (WHATEVER_STATE_NOT_NULL(state)) {
> 		/* not both are allowed at the same time */
> 		return NT_STATUS_INVALID_PARAMETER;
> 	}

	if (NOT_BOTH_AT_THE_SAME_TIME(state)) {
		return NT_STATUS_INVALID_PARAMETER;
	}

Doen't even need the comment anymore ...

> Which is clearly much more readable and even reusable.
> Often the problem is that there are such long statements in the first
> place. They are hard to read and easy to get wrong no matter where
> you place the opening brace and should be discouraged regardeless.
> 
> My 2c,
> Simo.
>