linux-kernel - Re: [PATCH 1/7] staging: rtl8723bs: wifi_regd.c: put spaces around binary operators

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1497446750.18751.50.camel@perches.com>
Date:   Wed, 14 Jun 2017 06:25:50 -0700
From:   Joe Perches <joe@...ches.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Ian W MORRISON <ianwmorrison@...il.com>
Cc:     Fabian Wolff <fabian.wolff@....de>,
        Máté Horváth <horvatmate@...il.com>,
        devel@...verdev.osuosl.org, linux-kernel@...cs.fau.de,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH 1/7] staging: rtl8723bs: wifi_regd.c: put spaces around
 binary operators

On Wed, 2017-06-14 at 15:19 +0300, Dan Carpenter wrote:
> On Wed, Jun 14, 2017 at 09:49:10PM +1000, Ian W MORRISON wrote:
> > On 14 June 2017 at 00:36, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> > > Kernel style is to have spaces around the operators.  This is staging
> > > code so we do all the style fixes.  We don't always update older drivers
> > > but sometimes we do.  No one is planning to change those drivers though
> > > so I guess the answer is no we're not going to update those unless you
> > > are?
> > > 
> > 
> > Thanks for the explanation. I assume submitting changes for the
> > drivers I identified would only be seen as minor corrections to 'the
> > chaff' resulting in unnecessary churn. If however it is expected that
> > corrections should be made when identified then I'm willing to prepare
> > a patch set. I'm happy to take advice either way.
> 
> I would just leave the old drivers as-is.

There would be some value in converting them, if only to
note what functions are identical across multiple drivers
and could be consolidated.

> Having spaces around operators has always been kernel style, but it's
> only fairly recently that checkpatch.pl started to complain.

Not really.  Linux is a relatively old project.

The CodingStyle for spaces around operators was added about
ten years ago.  Bit shifts were and still are not mentioned.

Messages for spaces around arithmetic were not emitted because
so much old code had various styles and it would have been a
lot of churn to update with many complaints from various
maintainers.  checkpatch today would emit about a quarter
million error messages for spacing on the kernel source tree.
That's a lot of whitespace churn.

There is a lot of code in drivers and a few other areas that
hasn't been touched in those ten years that doesn't follow
today's commonly accepted coding styles.

CodingStyle still doesn't mention a lot of style nits and bits.

About spacing around bit shifts:

This is just a --strict preference that several people had
expressed a desire to warn about spaces around arithmetic
and bit operators.  

It was added a couple years ago to checkpatch but not to
CodingStyle.

The --strict test applies by default to net/and drivers/net
because DaveM is pretty style conscious and asks for rework
when patches don't have the style he prefers.

And the --strict test applies to drivers/staging as well
because it gives more opportunities for first-time patch
submitters to send cleanup style patches (and GregKH can
be a stickler too).

> We keep making checkpatch.pl more stict as time goes on.

I try to add things to checkpatch only when there seems to
be a general consensus about a style or when a significant
percentage of code throughout the kernel already follows a 
specific style.

Actively developed Kernel code now has a fairly uniform
style and additions to checkpatch seem less necessary.

One area where checkpatch could use some enhancing is in
tracking indentation better.

checkpatch doesn't emit warnings with indentation like:

	statement(1);
		statement(2);

where the statements should be aligned on the same tabstop.

I'm playing with it but patches welcome...

> I think that's good
> because some reviewers will make you redo patches for style issues so
> having checkpatch.pl complain early on means you don't have redo the
> patch.  But it also means that old code will never be checkpatch.pl
> clean because we keep adding new checkpatch warnings.
> 
> And it's fine that old code has checkpatch warnings.  The point of
> checkpatch is to check new patches not to churn through old code.  As a
> reviewer, I find that checkpatch saves my time because I can often tell
> people to run it instead of listing all the style complaints.

All very true.