[00/13] nvdimm: Use more common kernel coding style
mbox series

Message ID cover.1568256705.git.joe@perches.com
Headers show
Series
  • nvdimm: Use more common kernel coding style
Related show

Message

Joe Perches Sept. 12, 2019, 2:54 a.m. UTC
Rather than have a local coding style, use the typical kernel style.

Joe Perches (13):
  nvdimm: Use more typical whitespace
  nvdimm: Move logical continuations to previous line
  nvdimm: Use octal permissions
  nvdimm: Use a more common kernel spacing style
  nvdimm: Use "unsigned int" in preference to "unsigned"
  nvdimm: Add and remove blank lines
  nvdimm: Use typical kernel brace styles
  nvdimm: Use typical kernel style indentation
  nvdimm: btt.h: Neaten #defines to improve readability
  nvdimm: namespace_devs: Move assignment operators
  nvdimm: Use more common logic testing styles and bare ; positions
  nvdimm: namespace_devs: Change progess typo to progress
  nvdimm: Miscellaneous neatening

 drivers/nvdimm/badrange.c       |  22 +-
 drivers/nvdimm/blk.c            |  39 ++--
 drivers/nvdimm/btt.c            | 249 +++++++++++----------
 drivers/nvdimm/btt.h            |  56 ++---
 drivers/nvdimm/btt_devs.c       |  68 +++---
 drivers/nvdimm/bus.c            | 138 ++++++------
 drivers/nvdimm/claim.c          |  50 ++---
 drivers/nvdimm/core.c           |  42 ++--
 drivers/nvdimm/dax_devs.c       |   3 +-
 drivers/nvdimm/dimm.c           |   3 +-
 drivers/nvdimm/dimm_devs.c      | 107 ++++-----
 drivers/nvdimm/e820.c           |   2 +-
 drivers/nvdimm/label.c          | 213 +++++++++---------
 drivers/nvdimm/label.h          |   6 +-
 drivers/nvdimm/namespace_devs.c | 472 +++++++++++++++++++++-------------------
 drivers/nvdimm/nd-core.h        |  31 +--
 drivers/nvdimm/nd.h             |  94 ++++----
 drivers/nvdimm/nd_virtio.c      |  20 +-
 drivers/nvdimm/of_pmem.c        |   6 +-
 drivers/nvdimm/pfn_devs.c       | 136 ++++++------
 drivers/nvdimm/pmem.c           |  57 ++---
 drivers/nvdimm/pmem.h           |   2 +-
 drivers/nvdimm/region.c         |  20 +-
 drivers/nvdimm/region_devs.c    | 160 +++++++-------
 drivers/nvdimm/security.c       | 138 ++++++------
 drivers/nvdimm/virtio_pmem.c    |  10 +-
 26 files changed, 1115 insertions(+), 1029 deletions(-)

Comments

Dan Williams Sept. 12, 2019, 8 a.m. UTC | #1
Hi Joe,

On Wed, Sep 11, 2019 at 7:55 PM Joe Perches <joe@perches.com> wrote:
>
> Rather than have a local coding style, use the typical kernel style.

I'd rather automate this. I'm going to do once-over with clang-format
and see what falls out.
Joe Perches Sept. 12, 2019, 8:15 a.m. UTC | #2
On Thu, 2019-09-12 at 01:00 -0700, Dan Williams wrote:
> Hi Joe,
> 
> On Wed, Sep 11, 2019 at 7:55 PM Joe Perches <joe@perches.com> wrote:
> > Rather than have a local coding style, use the typical kernel style.
> 
> I'd rather automate this. I'm going to do once-over with clang-format
> and see what falls out.

I am adding Miguel Ojeda to the cc's.

Of course you are welcome to try it, but I believe that
clang-format doesn't work all that well yet.

It's more a work in progress rather than a "standard".

I believe you'll find that the patch series I sent
ends up with a rather more typical kernel style.

I suggest you try to apply the series I sent and then
run clang-format on that and see the differences.

Ideally one day, something tool like clang-format
might be locally applied by every developer for their
own personal style with some other neutral style the
content actually distributed.

cheers, Joe
Miguel Ojeda Sept. 12, 2019, 8:42 a.m. UTC | #3
On Thu, Sep 12, 2019 at 10:15 AM Joe Perches <joe@perches.com> wrote:
>
> I am adding Miguel Ojeda to the cc's.

Thanks Joe!

> Of course you are welcome to try it, but I believe that
> clang-format doesn't work all that well yet.
>
> It's more a work in progress rather than a "standard".
>
> I believe you'll find that the patch series I sent
> ends up with a rather more typical kernel style.
>
> I suggest you try to apply the series I sent and then
> run clang-format on that and see the differences.

Indeed, it is not there just yet. There are a few differences w.r.t.
the kernel style that aren't supported yet. However, for block/batch
conversions, it is very useful.

Luckily, one of the biggest ones (the consecutive macros alignment,
and we have a lot of them given this is C and a kernel) is going away
with LLVM 9 which is about to be released next week.

> Ideally one day, something tool like clang-format
> might be locally applied by every developer for their
> own personal style with some other neutral style the
> content actually distributed.

If that day comes, I hope we can all agree to a single format and
apply it everywhere as other major projects have done. I think
agreeing to a given style is much, much easier for any of us when
formatting is fully automatic -- because at that point you don't need
to spend mental cycles (and memory!) on it. :-)

If I had to guess, I would say the path forward will start with some
subsystem maintainers starting to apply clang-format systematically on
their trees. That is why I think it is very useful that Dan tries it
out and let us know his impressions.

Cheers,
Miguel
Jeff Moyer Sept. 12, 2019, 2 p.m. UTC | #4
Joe Perches <joe@perches.com> writes:

> Rather than have a local coding style, use the typical kernel style.

The coding style isn't that different from the core kernel, and it's
still quite readable.  I'd rather avoid the churn and the risk of
introducing regressions.  This will also make backports to stable more
of a pain, so it isn't without cost.  Dan, is this really something you
want to do?

-Jeff

>
> Joe Perches (13):
>   nvdimm: Use more typical whitespace
>   nvdimm: Move logical continuations to previous line
>   nvdimm: Use octal permissions
>   nvdimm: Use a more common kernel spacing style
>   nvdimm: Use "unsigned int" in preference to "unsigned"
>   nvdimm: Add and remove blank lines
>   nvdimm: Use typical kernel brace styles
>   nvdimm: Use typical kernel style indentation
>   nvdimm: btt.h: Neaten #defines to improve readability
>   nvdimm: namespace_devs: Move assignment operators
>   nvdimm: Use more common logic testing styles and bare ; positions
>   nvdimm: namespace_devs: Change progess typo to progress
>   nvdimm: Miscellaneous neatening
>
>  drivers/nvdimm/badrange.c       |  22 +-
>  drivers/nvdimm/blk.c            |  39 ++--
>  drivers/nvdimm/btt.c            | 249 +++++++++++----------
>  drivers/nvdimm/btt.h            |  56 ++---
>  drivers/nvdimm/btt_devs.c       |  68 +++---
>  drivers/nvdimm/bus.c            | 138 ++++++------
>  drivers/nvdimm/claim.c          |  50 ++---
>  drivers/nvdimm/core.c           |  42 ++--
>  drivers/nvdimm/dax_devs.c       |   3 +-
>  drivers/nvdimm/dimm.c           |   3 +-
>  drivers/nvdimm/dimm_devs.c      | 107 ++++-----
>  drivers/nvdimm/e820.c           |   2 +-
>  drivers/nvdimm/label.c          | 213 +++++++++---------
>  drivers/nvdimm/label.h          |   6 +-
>  drivers/nvdimm/namespace_devs.c | 472 +++++++++++++++++++++-------------------
>  drivers/nvdimm/nd-core.h        |  31 +--
>  drivers/nvdimm/nd.h             |  94 ++++----
>  drivers/nvdimm/nd_virtio.c      |  20 +-
>  drivers/nvdimm/of_pmem.c        |   6 +-
>  drivers/nvdimm/pfn_devs.c       | 136 ++++++------
>  drivers/nvdimm/pmem.c           |  57 ++---
>  drivers/nvdimm/pmem.h           |   2 +-
>  drivers/nvdimm/region.c         |  20 +-
>  drivers/nvdimm/region_devs.c    | 160 +++++++-------
>  drivers/nvdimm/security.c       | 138 ++++++------
>  drivers/nvdimm/virtio_pmem.c    |  10 +-
>  26 files changed, 1115 insertions(+), 1029 deletions(-)
Johannes Thumshirn Sept. 12, 2019, 2:06 p.m. UTC | #5
On 12/09/2019 16:00, Jeff Moyer wrote:
> I'd rather avoid the churn and the risk of
> introducing regressions.  This will also make backports to stable more
> of a pain, so it isn't without cost.  Dan, is this really something you
> want to do?

I'm a 100% with Jeff on this!
Miguel Ojeda Sept. 12, 2019, 2:21 p.m. UTC | #6
On Thu, Sep 12, 2019 at 4:00 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Joe Perches <joe@perches.com> writes:
>
> > Rather than have a local coding style, use the typical kernel style.
>
> The coding style isn't that different from the core kernel, and it's
> still quite readable.  I'd rather avoid the churn and the risk of
> introducing regressions.  This will also make backports to stable more
> of a pain, so it isn't without cost.  Dan, is this really something you
> want to do?

+1 As soon as you get accustomed to have formatting done and enforced
automatically, it is great. Other major projects have done so for
quite a while now.

If doesn't think it is good enough, please let us know and, if it is
close enough, we can look at going for a newer LLVM to match the style
a bit more. Also note that one can disable formatting for some
sections of code if really needed.

Cheers,
Miguel
Dan Williams Sept. 12, 2019, 2:35 p.m. UTC | #7
On Thu, Sep 12, 2019 at 7:06 AM Johannes Thumshirn <jthumshirn@suse.de> wrote:
>
> On 12/09/2019 16:00, Jeff Moyer wrote:
> > I'd rather avoid the churn and the risk of
> > introducing regressions.  This will also make backports to stable more
> > of a pain, so it isn't without cost.  Dan, is this really something you
> > want to do?
>
> I'm a 100% with Jeff on this!

Agree, see my other response here:

https://lore.kernel.org/r/CAPcyv4iu13D5P+ExdeW8OGMV8g49fMUy52xbYZM+bewwVSwhjg@mail.gmail.com/
Joe Perches Sept. 12, 2019, 9:08 p.m. UTC | #8
On Thu, 2019-09-12 at 16:21 +0200, Miguel Ojeda wrote:

> As soon as you get accustomed to have formatting done and enforced
> automatically, it is great. Other major projects have done so for
> quite a while now.

Please name the major projects and then point to their
.clang-format equivalents.

Also note the size/scope/complexity of the major projects.

thanks.

> If doesn't think it is good enough, please let us know and, if it is
> close enough, we can look at going for a newer LLVM to match the style
> a bit more.

I used the latest one, and quite a bit of the conversion
was unpleasant to read.

>  Also note that one can disable formatting for some
> sections of code if really needed.

Marking sections _no_auto_format_ isn't really a
good solution is it?
.
Miguel Ojeda Sept. 12, 2019, 9:58 p.m. UTC | #9
On Thu, Sep 12, 2019 at 11:08 PM Joe Perches <joe@perches.com> wrote:
>
> Please name the major projects and then point to their
> .clang-format equivalents.
>
> Also note the size/scope/complexity of the major projects.

Mozilla, WebKit, LLVM and Microsoft. They have their style distributed
with the official clang-format, not sure if they enforce it.

Same for Chromium/Chrome, but it looks like they indeed enforce it:

  "A checkout should give you clang-format to automatically format C++
code. By policy, Clang's formatting of code should always be accepted
in code reviews."

I would bet other Google projects do so as well (since Chandler
Carruth has been giving talks about clang-format for 7+ years). Nick?

I hope those are major enough. There is also precedent in other
languages (e.g. Java, C#, Rust).

> I used the latest one, and quite a bit of the conversion
> was unpleasant to read.

It would be good to see particularly bad snippets to see if we can do
something about them (and, if needed, try to improve clang-format to
support whatever we need).

Did you tweak the parameters with the new ones? I am preparing an RFC
patch for an updated .clang-format configuration that improves quite a
bit the results w.r.t. to the current one (and allows for some leeway
on the developer's side, which helps prevent some cases too).

> Marking sections _no_auto_format_ isn't really a
> good solution is it?

I am thinking about special tables that are hand-crafted or very
complex macros. For those, yes, I think it is a fine solution. That is
why clang-format has that feature to begin with, and you can see an
example in Mozilla's style guide which points here:

  https://github.com/mozilla/gecko-dev/blob/master/xpcom/io/nsEscape.cpp#L22

Cheers,
Miguel
Joe Perches Sept. 12, 2019, 10:15 p.m. UTC | #10
On Thu, 2019-09-12 at 23:58 +0200, Miguel Ojeda wrote:
> On Thu, Sep 12, 2019 at 11:08 PM Joe Perches <joe@perches.com> wrote:
> > Please name the major projects and then point to their
> > .clang-format equivalents.
> > 
> > Also note the size/scope/complexity of the major projects.
> 
> Mozilla, WebKit, LLVM and Microsoft. They have their style distributed
> with the official clang-format, not sure if they enforce it.
> 
> Same for Chromium/Chrome, but it looks like they indeed enforce it:

thanks for that list.

> > I used the latest one, and quite a bit of the conversion
> > was unpleasant to read.
> 
> It would be good to see particularly bad snippets to see if we can do
> something about them (and, if needed, try to improve clang-format to
> support whatever we need).

As I mentioned earlier, look at the __stringify conversion.
Also the C() blocks.

btw: emacs 'mark-whole-buffer indent-region',
the tool I used for each file in patch 1, also
made a mess of the C() block.

> Did you tweak the parameters with the new ones?

No.  I used

$ clang-format --version
clang-format version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72)

and the existing .clang_format from
next-20190904 35394d031b710e832849fca60d0f53b513f0c390

> I am preparing an RFC
> patch for an updated .clang-format configuration that improves quite a
> bit the results w.r.t. to the current one (and allows for some leeway
> on the developer's side, which helps prevent some cases too).

Well, one day no doubt an automated tool will be
more useful for the kernel.  Hope you keep at it
and good luck.

> > Marking sections _no_auto_format_ isn't really a
> > good solution is it?
> 
> I am thinking about special tables that are hand-crafted or very
> complex macros. For those, yes, I think it is a fine solution. That is
> why clang-format has that feature to begin with, and you can see an
> example in Mozilla's style guide which points here:
> 
>   https://github.com/mozilla/gecko-dev/blob/master/xpcom/io/nsEscape.cpp#L22
> 
> Cheers,
> Miguel
Joe Perches Sept. 12, 2019, 10:38 p.m. UTC | #11
On Thu, 2019-09-12 at 23:58 +0200, Miguel Ojeda wrote:
> On Thu, Sep 12, 2019 at 11:08 PM Joe Perches <joe@perches.com> wrote:
> > Please name the major projects and then point to their
> > .clang-format equivalents.
> > 
> > Also note the size/scope/complexity of the major projects.
> 
> Mozilla, WebKit, LLVM and Microsoft. They have their style distributed
> with the official clang-format, not sure if they enforce it.

At least for LLVM, it appears not.

I just tried a very small portion of the clang compiler:

$ git ls-files llvm/lib/CodeGen/ | wc -l
293
$ git ls-files llvm/lib/CodeGen/ | xargs clang-format -i

and got:

$ git diff --shortstat
 245 files changed, 19519 insertions(+), 17794 deletions(-)

btw: that seems a pretty small ~7% of the overall lines

$ git ls-files llvm/lib/CodeGen/ | xargs wc -l | tail -1
 251034 total
Nick Desaulniers Sept. 12, 2019, 10:58 p.m. UTC | #12
On Thu, Sep 12, 2019 at 2:58 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Sep 12, 2019 at 11:08 PM Joe Perches <joe@perches.com> wrote:
> >
> > Please name the major projects and then point to their
> > .clang-format equivalents.
> >
> > Also note the size/scope/complexity of the major projects.
>
> Mozilla, WebKit, LLVM and Microsoft. They have their style distributed
> with the official clang-format, not sure if they enforce it.
>
> Same for Chromium/Chrome, but it looks like they indeed enforce it:
>
>   "A checkout should give you clang-format to automatically format C++
> code. By policy, Clang's formatting of code should always be accepted
> in code reviews."
>
> I would bet other Google projects do so as well (since Chandler
> Carruth has been giving talks about clang-format for 7+ years). Nick?

So Google3 (the internal monorepo that Android, Chromium, ChromiumOS,
Fuchsia are not a part of) is pretty sweet.  You cannot even post code
unless the linter has been run on it (presubmit hook), which for our
~350 millions LoC of C++ is clang-format.  If you bypass local
presubmit hooks, our code review tool ("critique") won't let you
submit code that fails lint presubmit checks.  I suspect the initial
conversion was probably committed by bots.

>
> I hope those are major enough. There is also precedent in other
> languages (e.g. Java, C#, Rust).

Yep! Other people coming to C/C++ from these languages find the
discussion about tabs vs spaces to be highly entertaining!  When you
have an automated code formatter and an agreed upon coding style (and
hopefully enforcement), you save so much time from avoided bikesheds!
Don't like the codebase's coding style?  Then write the code how you
like and just run the formatter when you're done (might not help with
conventions though, maybe that's where checkpatch.pl can shine).
Done! No more wasted time on what color to paint the bikeshed!
Nick Desaulniers Sept. 12, 2019, 11 p.m. UTC | #13
On Thu, Sep 12, 2019 at 3:38 PM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2019-09-12 at 23:58 +0200, Miguel Ojeda wrote:
> > On Thu, Sep 12, 2019 at 11:08 PM Joe Perches <joe@perches.com> wrote:
> > > Please name the major projects and then point to their
> > > .clang-format equivalents.
> > >
> > > Also note the size/scope/complexity of the major projects.
> >
> > Mozilla, WebKit, LLVM and Microsoft. They have their style distributed
> > with the official clang-format, not sure if they enforce it.
>
> At least for LLVM, it appears not.

I acknowledge the irony you present, but that's because there's no
enforcement on the LLVM side.  I frequently forget to run:
$ git-clang-format HEAD~

If you have automated systems that help encourage (ie. force) the use
of the formatter, this helps.

Consider the fact that not all kernel developers run checkpatch.pl.
Is that a deficiency in checkpatch.pl, or the lack of enforcement in
kernel developers' workflows?
Joe Perches Sept. 12, 2019, 11:07 p.m. UTC | #14
On Thu, 2019-09-12 at 16:00 -0700, Nick Desaulniers wrote:

> Consider the fact that not all kernel developers run checkpatch.pl.
> Is that a deficiency in checkpatch.pl, or the lack of enforcement in
> kernel developers' workflows?

No.  Mostly it's because the kernel is like a bunch of little
untethered development planets, each with a little prince that
wants to keep their own little fiefdom separate from the others.
Joe Perches Sept. 12, 2019, 11:26 p.m. UTC | #15
On Thu, 2019-09-12 at 23:58 +0200, Miguel Ojeda wrote:
> On Thu, Sep 12, 2019 at 11:08 PM Joe Perches <joe@perches.com> wrote:

> > Marking sections _no_auto_format_ isn't really a
> > good solution is it?
> 
> I am thinking about special tables that are hand-crafted or very
> complex macros. For those, yes, I think it is a fine solution.

Can the 'clang-format on/off' trigger be indirected into
something non-clang specific via a macro?

Not every project is going to use only the clang-format tool.
Miguel Ojeda Sept. 15, 2019, 6:25 p.m. UTC | #16
On Fri, Sep 13, 2019 at 1:26 AM Joe Perches <joe@perches.com> wrote:
>
> Not every project is going to use only the clang-format tool.

Why? The end goal would be to enforce all code to be running under the
same formatting rules (which, in practice, means the same tool at the
moment).

Note that you can use clang-format with most editors (including vim,
emacs, VS, VSCode, XCode, Sublime, Atom...).

Cheers,
Miguel