[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