[PATCH] Add utilities to check style

Simo simo at samba.org
Thu Apr 26 18:49:02 UTC 2018


On Thu, 2018-04-26 at 17:06 +0200, Ralph Böhme wrote:
> On Wed, Apr 25, 2018 at 09:26:12PM -0400, Simo wrote:
> > 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;
> >          }
> 
> heavens, no. I guess I got used to the README.Coding style, so it feels quite
> natural and the above just feels like two indentation errors in one line. :)
> 
> > 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.
> 
> Good luck inventing reusable macros for all the places with multi-line
> conditional expressions:
> 
> $ git grep -P -A 5 'if \((?!.*{)'
> 
> :)
> 
> > Often the problem is that there are such long statements in the first
> > place.
> 
> I guess that makes for a different README.Coding rule.

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 ?

Simo.



More information about the samba-technical mailing list