[PATCH] Add utilities to check style

[Simo]

On Tue, 2018-04-24 at 19:04 +1000, Martin Schwenke via samba-technical
wrote:
> 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.

I can do that.

> > > * 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.  :-(

Oh, I did not notice the warnings, I will fix that, I think the style
in the first case is just fine.

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

I always defined different helper functions in recent years:
#ifdef _PATH_LONG
bool check_nonblocking(const char *option)
{
	return strcmp(option, "nonblocking") == 0;
}
#else
bool check_nonblocking(const char *option)
{
	return false
}

and in the body:
[..]
	} else if check_nonblocking(option) {
		return true;
[..]

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

Thanks, will see if we can address that, easily, or will disable ...

Simo.

> peace & happiness,
> martin
>