[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