[PATCH] Add utilities to check style

Minor nit-pick isn't the max line length 80 characters.

Post by Simo via samba-technical
New revision of the patch to check style.
Martin I commented out the checks for the two behaviors you pointed
out.
Can I get a review so we can push this in and move to the next step ?
Simo.

Simo via samba-technical

2018-04-21 21:34:12 UTC

Post by Gary Lockyer via samba-technical

On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical

Post by Gary Lockyer via samba-technical
Minor nit-pick isn't the max line length 80 characters.

You are right, attached fixed patch.

Martin Schwenke via samba-technical

2018-04-23 06:13:18 UTC

On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical

Post by Gary Lockyer via samba-technical

On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical

Post by Gary Lockyer via samba-technical
Minor nit-pick isn't the max line length 80 characters.

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.

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

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

peace & happiness,
martin

Simo via samba-technical

2018-04-23 15:48:25 UTC

Post by Martin Schwenke via samba-technical
On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical

Post by Gary Lockyer via samba-technical

On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical

Post by Gary Lockyer via samba-technical
Minor nit-pick isn't the max line length 80 characters.

You are right, attached fixed patch.

A couple more comments, now that I've tested it on more changes... :-)
/*
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.

Post by Martin Schwenke via samba-technical
* 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 ?

Post by Martin Schwenke via samba-technical
* "Preprocessor statement in function body" is also interesting
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.

Post by Martin Schwenke via samba-technical
peace & happiness,
martin

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 ?

Simo.

Swen Schillig via samba-technical

2018-04-24 06:51:27 UTC

Post by Martin Schwenke via samba-technical
On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical
* 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,.
struct foo bar = {
data
}
struct foo bar =
{
data
}
do we ?

what about
struct foo bar = {0};

Cheers Swen

Simo via samba-technical

2018-04-24 17:30:27 UTC

Post by Swen Schillig via samba-technical

On Tue, 2018-04-24 at 08:51 +0200, Swen Schillig via samba-technical

I do not see why we want this to be a thing to be honest,.
struct foo bar = {
data
}
struct foo bar =
{
data
}
do we ?

what about
struct foo bar = {0};

What about it ?
Just to be clear, this check prevents a bare { opened on a line of its
own and not on the first column.
So.

int foobar(void)
{

is fine

while
for (;;)
{

is not.

I think this is the desired style we use in most code. There are some
instances where we used a code block hanging on its own in places, but
that was never sanctioned and I personally think it is bad style. It
was done before C99 when declaring variables could be done only at the
start of a code block and we wanted some variables to be local, but we
do not have that restriction anymore, and in either case, if you have
that need it really means you should have a helper function in there
anyway, rather than a new hanging code block.

HTH,
Simo.

Martin Schwenke via samba-technical

2018-04-24 09:04:49 UTC

Post by Martin Schwenke via samba-technical
On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical

[...]
[...]

Post by Simo via samba-technical
On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical

[...]

Post by Simo via samba-technical
You are right, attached fixed patch.

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

I do not see why we want this to be a thing to be honest,.
struct foo bar = {
data
}
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. :-(

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.

Post by Simo via samba-technical
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

Simo via samba-technical

2018-04-24 17:35:54 UTC

On Tue, 2018-04-24 at 19:04 +1000, Martin Schwenke via samba-technical

Post by Martin Schwenke via samba-technical
On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical

[...]
[...]

Post by Simo via samba-technical
On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical

[...]

Post by Simo via samba-technical
You are right, attached fixed patch.

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

I can do that.

I do not see why we want this to be a thing to be honest,.
struct foo bar = {
data
}
struct foo bar =
{
data
}
do we ?

struct foo bar[] = {
{ "a", 1 },
{ "b", 2 },
};
The above generates extra warnings.
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.

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;
[..]

Yeah, above. :-)

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

Simo.

Post by Martin Schwenke via samba-technical
peace & happiness,
martin

Simo via samba-technical

2018-04-24 20:56:59 UTC

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

Post by Martin Schwenke via samba-technical
On Sat, 21 Apr 2018 17:34:12 -0400, Simo via samba-technical

[...]
[...]

Post by Simo via samba-technical
On Sun, 2018-04-22 at 09:25 +1200, Gary Lockyer via samba-technical

[...]

Post by Simo via samba-technical
You are right, attached fixed patch.

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

I do not see why we want this to be a thing to be honest,.
struct foo bar = {
data
}
struct foo bar =
{
data
}
do we ?

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.

Yeah, above. :-)
peace & happiness,
martin

Jeremy Allison via samba-technical

2018-04-24 21:03:06 UTC

Post by Simo via samba-technical
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).
for (longline;
longline;
longline)
{
should be used instead of always keeping the opening on the same lne as
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 :-)

+1 from me on the second-form only. I much prefer the brace on the
opening line rather than the line below.

Ralph Böhme via samba-technical

2018-04-25 05:58:19 UTC

+1 from me on the second-form only. I much prefer the brace on the
opening line rather than the line below.

-1. The same applies to multiline if statements where we require the first form
for quite some time.

The example in README.Coding doesn't give a good example of why the second form
gets ugly for the case the "longline" is really a long line.

Example from source3/winbindd/winbindd_pam.c:

if (NT_STATUS_IS_OK(result)
&& (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
&& lp_winbind_offline_logon()) {
result = winbindd_update_creds_by_name(contact_domain, user,
newpass);

Putting the opening brace on a seperate line makes it easier to spot the if
clause condition vs the if body:

if (NT_STATUS_IS_OK(result)
&& (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
&& lp_winbind_offline_logon())
{
result = winbindd_update_creds_by_name(contact_domain, user,
newpass);

Example from source3/smbd/smb2_create.c that gets it right:

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;
}

Written with the opening brace on the same 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;
}

In this form your eyes start scanning for the brace to seperate the if condition
from the body while when written correctly you can immediately seperate both.

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Simo via samba-technical

2018-04-26 01:26:12 UTC

On Wed, 2018-04-25 at 07:58 +0200, Ralph Böhme via samba-technical

+1 from me on the second-form only. I much prefer the brace on the
opening line rather than the line below.

-1. The same applies to multiline if statements where we require the first form
for quite some time.
The example in README.Coding doesn't give a good example of why the second form
gets ugly for the case the "longline" is really a long line.
if (NT_STATUS_IS_OK(result)
&& (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
&& lp_winbind_offline_logon()) {
result = winbindd_update_creds_by_name(contact_domain, user,
newpass);
Putting the opening brace on a seperate line makes it easier to spot the if
if (NT_STATUS_IS_OK(result)
&& (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
&& lp_winbind_offline_logon())
{
result = winbindd_update_creds_by_name(contact_domain, user,
newpass);
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;
}
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;
}
In this form your eyes start scanning for the brace to seperate the if condition
from the body while when written correctly you can immediately seperate both.

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;
}

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.
Often the problem is that there are such long statements in the first
place. They are hard to read and easy to get wrong no matter where
you place the opening brace and should be discouraged regardeless.

My 2c,
Simo.

Simo via samba-technical

2018-04-26 01:34:08 UTC

Post by Simo via samba-technical
On Wed, 2018-04-25 at 07:58 +0200, Ralph Böhme via samba-technical

+1 from me on the second-form only. I much prefer the brace on the
opening line rather than the line below.

-1. The same applies to multiline if statements where we require the first form
for quite some time.
The example in README.Coding doesn't give a good example of why the second form
gets ugly for the case the "longline" is really a long line.
if (NT_STATUS_IS_OK(result)
&& (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
&& lp_winbind_offline_logon()) {
result = winbindd_update_creds_by_name(contact_domain, user,
newpass);
Putting the opening brace on a seperate line makes it easier to spot the if
if (NT_STATUS_IS_OK(result)
&& (state->request->flags & WBFLAG_PAM_CACHED_LOGIN)
&& lp_winbind_offline_logon())
{
result = winbindd_update_creds_by_name(contact_domain, user,
newpass);
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;
}
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;
}
In this form your eyes start scanning for the brace to seperate the if condition
from the body while when written correctly you can immediately seperate both.

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;
}
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)

Clearly not same semantics, but it doesn't change the point :-)

Corrected:
#define NOT_BOTH_AT_THE_SAME_TIME(s) \
((s->dhnc && s->dh2c) || \
(s->dhnc && s->dh2q) || \
(s->dh2c && s->dhnq) || \
(s->dh2q && s->dh2c))

Post by Simo via samba-technical
if (WHATEVER_STATE_NOT_NULL(state)) {
/* not both are allowed at the same time */
return NT_STATUS_INVALID_PARAMETER;
}

if (NOT_BOTH_AT_THE_SAME_TIME(state)) {
return NT_STATUS_INVALID_PARAMETER;
}

Doen't even need the comment anymore ...

Post by Simo via samba-technical
Which is clearly much more readable and even reusable.
Often the problem is that there are such long statements in the first
place. They are hard to read and easy to get wrong no matter where
you place the opening brace and should be discouraged regardeless.
My 2c,
Simo.

Ralph Böhme via samba-technical

2018-04-26 15:06:33 UTC

Post by Ralph BÃ¶hme via samba-technical
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. :)

Post by Ralph BÃ¶hme via samba-technical
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)
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 \((?!.*{)'

:)

Post by Ralph BÃ¶hme via samba-technical
Often the problem is that there are such long statements in the first
place.

I guess that makes for a different README.Coding rule.

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Simo via samba-technical

2018-04-26 18:49:02 UTC

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

Good luck inventing reusable macros for all the places with multi-line
$ git grep -P -A 5 'if \((?!.*{)'
:)

Post by Ralph BÃ¶hme via samba-technical
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.

Jeremy Allison via samba-technical

2018-04-26 18:54:48 UTC

Post by Simo via samba-technical
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 ?

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Simo via samba-technical

2018-04-26 19:31:32 UTC

On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.

Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

Simo.

Simo via samba-technical

2018-05-03 13:41:42 UTC

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

Bump,

Jeremy, Ralph,
is this version ok ?
If so can you RB and push to autobuild ?

Simo.

Ralph Böhme via samba-technical

2018-05-03 13:54:18 UTC

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

Bump,
Jeremy, Ralph,
is this version ok ?
If so can you RB and push to autobuild ?

sorry. I'll give it a whirl later on with my largish WIP persistent handles
branches and will report back.

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Ralph Böhme via samba-technical

2018-05-03 17:46:28 UTC

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

Bump,
Jeremy, Ralph,
is this version ok ?

with commit 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44 from this branch:
<https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/ph-smb>

I'm getting a fatal error:

$ script/cstyle.py 54f124f39bc~1..54f124f39bc
54f124f39bc dbwrap: add flags arg to dbwrap_fetch_locked() and dbwrap_try_fetch_locked()
Unexpected command failure:
UnicodeDecodeError(... , 161, 162, 'invalid continuation byte')

I have no idea what triggers this. Can you check?

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Simo via samba-technical

2018-05-06 14:56:59 UTC

On Thu, 2018-05-03 at 19:46 +0200, Ralph Böhme via samba-technical

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

Bump,
Jeremy, Ralph,
is this version ok ?

<https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/ph-smb>
$ script/cstyle.py 54f124f39bc~1..54f124f39bc
54f124f39bc dbwrap: add flags arg to dbwrap_fetch_locked() and dbwrap_try_fetch_locked()
UnicodeDecodeError(... , 161, 162, 'invalid continuation byte')
I have no idea what triggers this. Can you check?

What python version are you using ?

Simo.

Ralph Böhme via samba-technical

2018-05-06 15:33:23 UTC

Post by Simo via samba-technical
What python version are you using ?

[***@kazak scratch]$ cat /etc/fedora-release
Fedora release 26 (Twenty Six)

[***@kazak scratch]$ python --version
Python 2.7.14

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Simo via samba-technical

2018-05-06 17:22:05 UTC

On Thu, 2018-05-03 at 19:46 +0200, Ralph Böhme via samba-technical

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

Bump,
Jeremy, Ralph,
is this version ok ?

I fetched this branch but there is
no 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44

so I can't test it.
Simo.

Ralph Böhme via samba-technical

2018-05-06 19:54:48 UTC

Post by Simo via samba-technical
I fetched this branch but there is
no 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44
so I can't test it.

hm, maybe I forgot to push the snapshot of the branch. The branch "tmp" now
points at commit 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44.

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Simo via samba-technical

2018-05-07 12:11:43 UTC

Post by Simo via samba-technical
I fetched this branch but there is
no 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44
so I can't test it.

hm, maybe I forgot to push the snapshot of the branch. The branch "tmp" now
points at commit 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44.

Thanks,
If my coffee is strong enough this morning :-) I think the error is
here: "Jean Fran\xe7ois Micouleau"

It think this has been copied over and over since back then when code
pages were used instead of utf8.

There are 6 files in the tree with invalid utf8 characters in Jean
FranÃ§ois name. A patch is attached to fix that.

Please RB+ and push this patch, first win for the style checker,
although it found an encoding issue rather than a style one :-)

Simo.

Ralph Böhme via samba-technical

2018-05-07 13:47:18 UTC

Post by Steve French via samba-technical

Post by Simo via samba-technical
I fetched this branch but there is
no 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44
so I can't test it.

hm, maybe I forgot to push the snapshot of the branch. The branch "tmp" now
points at commit 54f124f39bc5ae316faab97e2ee2d00c3b8cfe44.

Thanks,
If my coffee is strong enough this morning :-) I think the error is
here: "Jean Fran\xe7ois Micouleau"
It think this has been copied over and over since back then when code
pages were used instead of utf8.
There are 6 files in the tree with invalid utf8 characters in Jean
François name. A patch is attached to fix that.
Please RB+ and push this patch, first win for the style checker,
although it found an encoding issue rather than a style one :-)

:)

Pushed. Thanks!

I'll retest my WIP branches with the style checker one this fix is in master.

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Martin Schwenke via samba-technical

2018-05-04 04:02:07 UTC

On Thu, 26 Apr 2018 15:31:32 -0400, Simo via samba-technical

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

I think this is good, useful and very close, so we should get it into
the tree because it is useful. So:

Reviewed-by: Martin Schwenke <***@meltin.net>

It still see minor issues and will argue against including this in
tests until it more closely reflects README.Coding.

Although it doesn't produce a warning or exit non-zero, the indent
output makes this recommendation:

ctdb->notification_script = talloc_asprintf(ctdb,
- "%s/notify.sh",
- ctdb_base);
+ "%s/notify.sh", ctdb_base);

This contradicts the change to README.Coding made in commit
614f5a041ea6e021c2ff9c4e462b0e22626c7f33. I don't think there's a way
of telling indent to leave such lines alone. :-( Perhaps we should
avoid printing the indent output?

Speaking of that change to README.Coding, I would like to see it
changed to a recommendation rather than a a requirement. Making code
take up needless vertical space makes it harder to read the code around
it.

If you look at a logging call that includes several short variables but
where the format string is enough long to cause a line break then you
can end up with something that is horrible to read. Here's a
made-up example:

DBG_INFO("Failed in iteration %u for lmaster=%u, dmaster=%u (%s)\n",
i,
state->lmaster,
state->dmaster,
strerror(ret));

This could easily fit on 2 lines instead of dominating the code around
it.

I think that for function calls we're better off letting people use
their discretion.

Sorry I didn't get a chance to complain about this at the time that
change was proposed. :-(

peace & happiness,
martin

Ralph Böhme via samba-technical

2018-05-04 07:11:49 UTC

Hi Martin,

Post by Martin Schwenke via samba-technical
On Thu, 26 Apr 2018 15:31:32 -0400, Simo via samba-technical

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

You know what they say about opinions :-). Yeah, I guess
this is just a style thing - you and I like it one way,
Ralph likes another. In those cases we should probably
allow both IMHO.

Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

I think this is good, useful and very close, so we should get it into

please don't push before the issue I ran into is sorted out. I couldn't really
test and review the patch due to that.

Post by Martin Schwenke via samba-technical
It still see minor issues and will argue against including this in
tests until it more closely reflects README.Coding.
Although it doesn't produce a warning or exit non-zero, the indent
ctdb->notification_script = talloc_asprintf(ctdb,
- "%s/notify.sh",
- ctdb_base);
+ "%s/notify.sh", ctdb_base);
This contradicts the change to README.Coding made in commit
614f5a041ea6e021c2ff9c4e462b0e22626c7f33. I don't think there's a way
of telling indent to leave such lines alone. :-( Perhaps we should
avoid printing the indent output?

Hm. Imho the script must support our recommended way of indenting functions and
macros. If it doesn't support that we can't really use it.

Post by Martin Schwenke via samba-technical
Speaking of that change to README.Coding, I would like to see it
changed to a recommendation rather than a a requirement. Making code
take up needless vertical space makes it harder to read the code around
it.

The idea of the rules in README.Coding is to result in better readable code. As
every rule can easily degrade when applied without thinking, imho the rules
should be recommendations and not requirements anyway.

Post by Martin Schwenke via samba-technical
If you look at a logging call that includes several short variables but
where the format string is enough long to cause a line break then you
can end up with something that is horrible to read. Here's a
DBG_INFO("Failed in iteration %u for lmaster=%u, dmaster=%u (%s)\n",
i,
state->lmaster,
state->dmaster,
strerror(ret));

Iirc README.Coding doesn't mention macros, so we could keep the rule for
function calls but relax it for macros. I fully agree that those multine debug
macros are a nuisance and a perfect example how a sane rule degrades it
something not so sane. :)

Post by Martin Schwenke via samba-technical
This could easily fit on 2 lines instead of dominating the code around
it.
I think that for function calls we're better off letting people use
their discretion.

The idea was to minimize diffs of future changes that add or remove function
parameters. The one-arg-per-line format produces well readable code and applying
the rule is simple and doesn't require reshuffling to adhere to the 80 lines
reule while WIP code evolves. That's the no 1 reason why I like it. :)

-slow

--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
59E4 AA1E 9B71 2639 9E46

Martin Schwenke via samba-technical

2018-05-04 10:35:08 UTC

Hi Ralph,

Post by Martin Schwenke via samba-technical
On Thu, 26 Apr 2018 15:31:32 -0400, Simo via samba-technical

Post by Simo via samba-technical
On Thu, 2018-04-26 at 11:54 -0700, Jeremy Allison via samba-technical

[...]
[...]
[...]

Post by Simo via samba-technical
Ok, I am going to strip any checking for parens then
because both == no stlye == no check we can enforce.
Attached find a patch with the check disabled (but the code is changed
to reflect my last proposal, so if we change mind at least we have the
code ready to be used).

I think this is good, useful and very close, so we should get it into

please don't push before the issue I ran into is sorted out. I couldn't really
test and review the patch due to that.

Darn. Sorry, I forgot that I'd seen that and got too keen. :-(

Hm. Imho the script must support our recommended way of indenting
functions and macros. If it doesn't support that we can't really use
it.

True, but if it helps to find useful issues then it is better than
nothing.

At the moment both warnings and the final diff is printed to stdout.
One possibility could be to make the diff optional, so that we can just
concentrate on the warnings. Alternatively, we could send the warning
to stderr and the diff to stdout... and potentially ignore the
diff. :-)

Post by Martin Schwenke via samba-technical
Speaking of that change to README.Coding, I would like to see it
changed to a recommendation rather than a a requirement. Making
code take up needless vertical space makes it harder to read the
code around it.

The idea of the rules in README.Coding is to result in better
readable code. As every rule can easily degrade when applied without
thinking, imho the rules should be recommendations and not
requirements anyway.

I think some of the simple, well-defined things (e.g. untidy whitespace)
can always be requirements. If we're going to do any automatic checking
when we need to distinguish between requirements and recommendations...

Post by Martin Schwenke via samba-technical
If you look at a logging call that includes several short variables
but where the format string is enough long to cause a line break
then you can end up with something that is horrible to read.
DBG_INFO("Failed in iteration %u for lmaster=%u, dmaster=%u
(%s)\n", i,
state->lmaster,
state->dmaster,
strerror(ret));

Iirc README.Coding doesn't mention macros, so we could keep the rule
for function calls but relax it for macros. I fully agree that those
multine debug macros are a nuisance and a perfect example how a sane
rule degrades it something not so sane. :)

Yeah, I wondered about that... :-)

... but then you get to printf()/fprintf() and you find yourself on a
slippery slope!

Post by Martin Schwenke via samba-technical
This could easily fit on 2 lines instead of dominating the code
around it.
I think that for function calls we're better off letting people use
their discretion.

The idea was to minimize diffs of future changes that add or remove
function parameters. The one-arg-per-line format produces well
readable code and applying the rule is simple and doesn't require
reshuffling to adhere to the 80 lines reule while WIP code evolves.
That's the no 1 reason why I like it. :)

Well, we need to balance overall readability of code against minimising
diffs.

I like it in a lot of cases too. However, I think that there are cases
where a simple function call could easily grow to 6 or 7 lines and
dominate more important code around it. If unnecessary vertical space
in a couple of function calls stops me from being able to fit a function
on my screen, that makes the code harder to read and understanding, and
it is having a negative overall effect.

... and the bike shed definitely has to be purple! ;-)

peace & happiness,
martin

Steve French via samba-technical

2018-04-23 15:43:29 UTC

I have been impressed with scripts/checkpatch.pl (other than it is
perl and 10 times larger - it covers a lot more common errors and
style nits). https://wintergreenworks.wordpress.com/2012/05/30/checkpatch-linux-coding-style-check/
In addition, sparse has been invaluable in catching endian errors and
32-->64 bit overflow issues and lots of common C errors. Has anyone
looked at using these for Samba?

On Sat, Apr 21, 2018 at 4:08 PM, Simo via samba-technical

--
Thanks,

Steve

Simo via samba-technical

2018-04-23 15:55:40 UTC

Well,
so here my assesment:

it is perl,
it is very kernel specific,
also it is perl,
has checks for a lot of things we really do not care for,
also... it is perl,
it seem more concerned about patch submission than style,
and it is also perl ...

So I do not know if this tool would be really valuable, but it is
perl, and I am certainly not going to relearn that "thing" :-)

Simo.

On Mon, 2018-04-23 at 10:43 -0500, Steve French via samba-technical

Post by Steve French via samba-technical
I have been impressed with scripts/checkpatch.pl (other than it is
perl and 10 times larger - it covers a lot more common errors and
style nits). https://wintergreenworks.wordpress.com/2012/05/30/checkpatch-linux-coding-style-check/
In addition, sparse has been invaluable in catching endian errors and
32-->64 bit overflow issues and lots of common C errors. Has anyone
looked at using these for Samba?
On Sat, Apr 21, 2018 at 4:08 PM, Simo via samba-technical

Ralph Böhme via samba-technical

2018-04-25 06:05:34 UTC