[PATCH] Add utilities to check style

[Simo]

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)

then that if turns into:

	if (WHATEVER_STATE_NOT_NULL(state)) {
		/* not both are allowed at the same time */
		return NT_STATUS_INVALID_PARAMETER;
	}

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.