| Message ID | cover.1568256705.git.joe@perches.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | nvdimm: Use more common kernel coding style | expand |
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.
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
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
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(-)
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!
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
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/
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? .
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
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
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
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!
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?
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.
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.
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