[PATCH] Add utilities to check style

[Simo]

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

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.
> 
> > > * 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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-utilities-to-check-style.patch
Type: text/x-patch
Size: 21610 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180424/247d2a7b/0001-Add-utilities-to-check-style.bin>