[PATCH] Add utilities to check style

Martin Schwenke martin at meltin.net
Tue Apr 24 09:04:49 UTC 2018


On Mon, 23 Apr 2018 11:48:25 -0400, Simo <simo at samba.org> wrote:

> On Mon, 2018-04-23 at 16:13 +1000, Martin Schwenke wrote:
> > On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical
> > <samba-technical at lists.samba.org> wrote:
> >   
>  [...]  
>  [...]  
> > > On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical
> > > wrote:  
>  [...]  
> > > You are right, attached fixed patch.  
> > 
> > A couple more comments, now that I've tested it on more changes...  :-)
> > 
> > * It doesn't like comments like this:
> > 
> >   /*
> >      Hello world
> >   */
> > 
> >   Either do I!  :-)  However, README.Coding doesn't say anything about
> >   this and we have plenty of them in the code.  Not sure what to
> >   suggest here.  
> 
> Unless we have a strong desire to allow those, I would just switch to
> the more consistent checks this tool performs.
> More checks are not necessarily bad unless we have an explicit reason
> to dislike a more specific style check.
> More consistency is always a good thing in my book, more freedom is ok
> only when there is a good reason to allow it.

I agree.  But then we should propose updates to READING.Coding in
parallel.

> > * The "Un-cuddled open brace" thing is going to be annoying
> > 
> >   If you're initialising an array of structures then I don't see how
> >   you can avoid this warning.
> > 
> >   Similarly, when initialising a fairly long array of structures you
> >   might want to have the values inline, with opening and closing braces
> >   on the same line as the data.  However, this generates lots of
> >   complaints.  :-(  
> 
> I do not see why we want this to be a thing to be honest,.
> 
> We do:
>   struct foo bar = {
>     data
>   }
> We do not do:
>   struct foo bar = 
>   {
>      data
>   }
> do we ?

No, but we do:

struct foo bar[] = {
	{ "a", 1 },
	{ "b", 2 },
};

The above generates extra warnings.

However, even if you do:

struct foo bar[] = {
	{
		"a", 1
	},
	{
		"b", 2 
	},
};

(which gets unwieldy for large initialisations) those opening braces
are still un-cuddled... though the other warnings go away.  :-(

> > * "Preprocessor statement in function body" is also interesting
> > 
> >   We have:
> > 
> > static bool syslog_log_validate(const char *option)
> > {
> >         if (option == NULL) {
> >                 return true;
> > #ifdef _PATH_LOG
> >         } else if (strcmp(option, "nonblocking") == 0) {
> >                 return true;
> > #endif
> >         } else if (strcmp(option, "udp") == 0) {
> >                 return true;
> >         } else if (strcmp(option, "udp-rfc5424") == 0) {
> >                 return true;
> >         }
> > 
> >         return false;
> > }
> > 
> >   This seems like a sensible thing to do...  
> 
> Tend do be horrible for readbility and also source of code flow errors,
> but if we *really* want that I guess we can disable that check too.

I think it is OK for short functions.  I'm not sure how else you would
do it, apart from defining different functions depending on whether
_PATH_LOG is defined.

> Attached new patch that disables also preprocessor statement checks,
> but keeps the rest. including un-cuddled errors. Can you show me an
> example where the style using a bare braces is desirable ?

Yeah, above.  :-)

peace & happiness,
martin



More information about the samba-technical mailing list