[PATCH] Add utilities to check style
Ralph Böhme
slow at samba.org
Wed Apr 25 05:58:19 UTC 2018
On Tue, Apr 24, 2018 at 02:03:06PM -0700, Jeremy Allison via samba-technical wrote:
> On Tue, Apr 24, 2018 at 04:56:59PM -0400, Simo via samba-technical wrote:
> > 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 :-)
>
> +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
More information about the samba-technical
mailing list