linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] .clang-format: Remove conditional comments
@ 2020-11-03 18:29 Joe Perches
  2020-11-03 21:33 ` Miguel Ojeda
  2020-11-04  1:11 ` Nick Desaulniers
  0 siblings, 2 replies; 17+ messages in thread
From: Joe Perches @ 2020-11-03 18:29 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: clang-built-linux, LKML

Now that the clang minimum supported version is > 10.0, enable the
commented out conditional reformatting key:value lines in the file.

Signed-off-by: Joe Perches <joe@perches.com>
---

Hey Miguel.

I don't use this, but on its face it seems a reasonable change
if the commented out key:value lines are correct.

 .clang-format | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/.clang-format b/.clang-format
index 10dc5a9a61b3..531b97501f14 100644
--- a/.clang-format
+++ b/.clang-format
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 #
-# clang-format configuration file. Intended for clang-format >= 4.
+# clang-format configuration file. Intended for clang-format >= 10.
 #
 # For more information, see:
 #
@@ -13,7 +13,7 @@ AccessModifierOffset: -4
 AlignAfterOpenBracket: Align
 AlignConsecutiveAssignments: false
 AlignConsecutiveDeclarations: false
-#AlignEscapedNewlines: Left # Unknown to clang-format-4.0
+AlignEscapedNewlines: Left
 AlignOperands: true
 AlignTrailingComments: false
 AllowAllParametersOfDeclarationOnNextLine: false
@@ -37,24 +37,24 @@ BraceWrapping:
   AfterObjCDeclaration: false
   AfterStruct: false
   AfterUnion: false
-  #AfterExternBlock: false # Unknown to clang-format-5.0
+  AfterExternBlock: false
   BeforeCatch: false
   BeforeElse: false
   IndentBraces: false
-  #SplitEmptyFunction: true # Unknown to clang-format-4.0
-  #SplitEmptyRecord: true # Unknown to clang-format-4.0
-  #SplitEmptyNamespace: true # Unknown to clang-format-4.0
+  SplitEmptyFunction: true
+  SplitEmptyRecord: true
+  SplitEmptyNamespace: true
 BreakBeforeBinaryOperators: None
 BreakBeforeBraces: Custom
-#BreakBeforeInheritanceComma: false # Unknown to clang-format-4.0
+BreakBeforeInheritanceComma: false
 BreakBeforeTernaryOperators: false
 BreakConstructorInitializersBeforeComma: false
-#BreakConstructorInitializers: BeforeComma # Unknown to clang-format-4.0
+BreakConstructorInitializers: BeforeComma
 BreakAfterJavaFieldAnnotations: false
 BreakStringLiterals: false
 ColumnLimit: 80
 CommentPragmas: '^ IWYU pragma:'
-#CompactNamespaces: false # Unknown to clang-format-4.0
+CompactNamespaces: false
 ConstructorInitializerAllOnOneLineOrOnePerLine: false
 ConstructorInitializerIndentWidth: 8
 ContinuationIndentWidth: 8
@@ -62,7 +62,7 @@ Cpp11BracedListStyle: false
 DerivePointerAlignment: false
 DisableFormat: false
 ExperimentalAutoDetectBinPacking: false
-#FixNamespaceComments: false # Unknown to clang-format-4.0
+FixNamespaceComments: false
 
 # Taken from:
 #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ \
@@ -494,13 +494,13 @@ ForEachMacros:
   - 'xbc_node_for_each_key_value'
   - 'zorro_for_each_dev'
 
-#IncludeBlocks: Preserve # Unknown to clang-format-5.0
+IncludeBlocks: Preserve
 IncludeCategories:
   - Regex: '.*'
     Priority: 1
 IncludeIsMainRegex: '(Test)?$'
 IndentCaseLabels: false
-#IndentPPDirectives: None # Unknown to clang-format-5.0
+IndentPPDirectives: None
 IndentWidth: 8
 IndentWrappedFunctionNames: false
 JavaScriptQuotes: Leave
@@ -510,13 +510,13 @@ MacroBlockBegin: ''
 MacroBlockEnd: ''
 MaxEmptyLinesToKeep: 1
 NamespaceIndentation: None
-#ObjCBinPackProtocolList: Auto # Unknown to clang-format-5.0
+ObjCBinPackProtocolList: Auto
 ObjCBlockIndentWidth: 8
 ObjCSpaceAfterProperty: true
 ObjCSpaceBeforeProtocolList: true
 
 # Taken from git's rules
-#PenaltyBreakAssignment: 10 # Unknown to clang-format-4.0
+PenaltyBreakAssignment: 10
 PenaltyBreakBeforeFirstCallParameter: 30
 PenaltyBreakComment: 10
 PenaltyBreakFirstLessLess: 0
@@ -527,14 +527,14 @@ PenaltyReturnTypeOnItsOwnLine: 60
 PointerAlignment: Right
 ReflowComments: false
 SortIncludes: false
-#SortUsingDeclarations: false # Unknown to clang-format-4.0
+SortUsingDeclarations: false
 SpaceAfterCStyleCast: false
 SpaceAfterTemplateKeyword: true
 SpaceBeforeAssignmentOperators: true
-#SpaceBeforeCtorInitializerColon: true # Unknown to clang-format-5.0
-#SpaceBeforeInheritanceColon: true # Unknown to clang-format-5.0
+SpaceBeforeCtorInitializerColon: true
+SpaceBeforeInheritanceColon: true
 SpaceBeforeParens: ControlStatements
-#SpaceBeforeRangeBasedForLoopColon: true # Unknown to clang-format-5.0
+SpaceBeforeRangeBasedForLoopColon: true
 SpaceInEmptyParentheses: false
 SpacesBeforeTrailingComments: 1
 SpacesInAngles: false


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-03 18:29 [RFC PATCH] .clang-format: Remove conditional comments Joe Perches
@ 2020-11-03 21:33 ` Miguel Ojeda
  2020-11-04  0:56   ` Joe Perches
  2020-11-04  1:08   ` Nick Desaulniers
  2020-11-04  1:11 ` Nick Desaulniers
  1 sibling, 2 replies; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-03 21:33 UTC (permalink / raw)
  To: Joe Perches; +Cc: clang-built-linux, LKML

Hi Joe,

On Tue, Nov 3, 2020 at 7:29 PM Joe Perches <joe@perches.com> wrote:
>
> Now that the clang minimum supported version is > 10.0, enable the
> commented out conditional reformatting key:value lines in the file.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> Hey Miguel.
>
> I don't use this, but on its face it seems a reasonable change
> if the commented out key:value lines are correct.

It is, yeah; however, the concern is that there may be developers
running an old clang-format from their distro (i.e. not using it for
compiling the kernel). We need to compare the functionality advantage
vs. the inconvenience of installing a current LLVM. The best would be
to ask whoever is using it right now, but there is no easy way to do
that -- many will only notice when the change is actually pushed :-)

So far, I have avoided upgrading the requirement until clang-format
could match the kernel style even better (i.e. so that when the
upgrade happens, there is a reason for it). Also, the configuration
can be overridden in subfolders, thus a maintainer can push things
forward in a subsystem meanwhile.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-03 21:33 ` Miguel Ojeda
@ 2020-11-04  0:56   ` Joe Perches
  2020-11-04  3:10     ` Miguel Ojeda
  2020-11-04  1:08   ` Nick Desaulniers
  1 sibling, 1 reply; 17+ messages in thread
From: Joe Perches @ 2020-11-04  0:56 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: clang-built-linux, LKML

On Tue, 2020-11-03 at 22:33 +0100, Miguel Ojeda wrote:
> It is, yeah; however, the concern is that there may be developers
> running an old clang-format from their distro (i.e. not using it for
> compiling the kernel).

I expect that'd be extremely unusual.

> We need to compare the functionality advantage
> vs. the inconvenience of installing a current LLVM. The best would be
> to ask whoever is using it right now, but there is no easy way to do
> that -- many will only notice when the change is actually pushed :-)

Do remember that this patch is for the current kernel and
not any old version that someone might be compiling.

The current kernel _requires_ clang 10.0+ and that would
obviously provide clang-format 10+ as well.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-03 21:33 ` Miguel Ojeda
  2020-11-04  0:56   ` Joe Perches
@ 2020-11-04  1:08   ` Nick Desaulniers
  2020-11-04  1:31     ` Joe Perches
  2020-11-04  3:40     ` Miguel Ojeda
  1 sibling, 2 replies; 17+ messages in thread
From: Nick Desaulniers @ 2020-11-04  1:08 UTC (permalink / raw)
  To: Miguel Ojeda, Joe Perches; +Cc: clang-built-linux, LKML

On Tue, Nov 3, 2020 at 1:33 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Hi Joe,
>
> On Tue, Nov 3, 2020 at 7:29 PM Joe Perches <joe@perches.com> wrote:
> >
> > Now that the clang minimum supported version is > 10.0, enable the
> > commented out conditional reformatting key:value lines in the file.
> >
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >
> > Hey Miguel.
> >
> > I don't use this, but on its face it seems a reasonable change
> > if the commented out key:value lines are correct.

Joe,
what would it take to get you to use clang-format, or at least try it?
 Beers?  Bribes? Dirty deeds, done dirt cheap?

> It is, yeah; however, the concern is that there may be developers
> running an old clang-format from their distro (i.e. not using it for
> compiling the kernel). We need to compare the functionality advantage
> vs. the inconvenience of installing a current LLVM. The best would be
> to ask whoever is using it right now, but there is no easy way to do
> that -- many will only notice when the change is actually pushed :-)
>
> So far, I have avoided upgrading the requirement until clang-format
> could match the kernel style even better (i.e. so that when the
> upgrade happens, there is a reason for it). Also, the configuration
> can be overridden in subfolders, thus a maintainer can push things
> forward in a subsystem meanwhile.

Miguel,
Really? :P I'd bet if you picked up this patch no one would notice.

I recommend a simpler approach to multiple version support, which is
just matching the one version recommended for the rest of LLVM tools.
Sure, technically you can use older tools, but do so at your own peril
and don't complain to us if it doesn't work.  Otherwise trying to
explain different versions and even for different directories gets way
too complex for anyone to take seriously.  It's not like we backport
raising the minimum version.

I was very much in denial of committing to a relatively high minimum
version of LLVM myself, until Linus recommended the simpler approach
which folks voted in favor of at Plumbers.  Maybe not a perfect
analogy though.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-03 18:29 [RFC PATCH] .clang-format: Remove conditional comments Joe Perches
  2020-11-03 21:33 ` Miguel Ojeda
@ 2020-11-04  1:11 ` Nick Desaulniers
  1 sibling, 0 replies; 17+ messages in thread
From: Nick Desaulniers @ 2020-11-04  1:11 UTC (permalink / raw)
  To: Joe Perches; +Cc: Miguel Ojeda, clang-built-linux, LKML

On Tue, Nov 3, 2020 at 10:29 AM Joe Perches <joe@perches.com> wrote:
>
> Now that the clang minimum supported version is > 10.0, enable the
> commented out conditional reformatting key:value lines in the file.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> Hey Miguel.
>
> I don't use this, but on its face it seems a reasonable change
> if the commented out key:value lines are correct.

Acked-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  1:08   ` Nick Desaulniers
@ 2020-11-04  1:31     ` Joe Perches
  2020-11-05  0:35       ` Nick Desaulniers
  2020-11-04  3:40     ` Miguel Ojeda
  1 sibling, 1 reply; 17+ messages in thread
From: Joe Perches @ 2020-11-04  1:31 UTC (permalink / raw)
  To: Nick Desaulniers, Miguel Ojeda; +Cc: clang-built-linux, LKML

On Tue, 2020-11-03 at 17:08 -0800, Nick Desaulniers wrote:
> On Tue, Nov 3, 2020 at 1:33 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote: 
> > On Tue, Nov 3, 2020 at 7:29 PM Joe Perches <joe@perches.com> wrote:
> > > 
> > > Now that the clang minimum supported version is > 10.0, enable the
> > > commented out conditional reformatting key:value lines in the file.
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > ---
> > > 
> > > Hey Miguel.
> > > 
> > > I don't use this, but on its face it seems a reasonable change
> > > if the commented out key:value lines are correct.
> 
> Joe,
> what would it take to get you to use clang-format, or at least try it?
>  Beers?  Bribes? Dirty deeds, done dirt cheap?

Hey Nick.

Paint my house? ;)

I've tried it.  It's OK.  It's not significantly better than
Lindent in some ways, in other ways it's pretty good.

It can make a real hash though of well formatted, human readable
code.  I think that's it's biggest drawback.

I posted an example of it a year or so back.

https://lore.kernel.org/lkml/e9cb9bc8bd7fe38a5bb6ff7b7222b512acc7b018.camel@perches.com/

In that thread I wrote:

On Thu, 2019-09-12 at 03:18 -0700, Joe Perches wrote:
> On Thu, 2019-09-12 at 10:24 +0200, Miguel Ojeda wrote:
> > On Thu, Sep 12, 2019 at 9:43 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > > Now I come to find that CodingStyle has settled on clang-format (in
> > > the last 15 months) as the new standard which is a much better answer
> > > to me than a manually specified style open to interpretation. I'll
> > > take a look at getting libnvdimm converted over.
> > 
> > Note that clang-format cannot do everything as we want within the
> > kernel just yet, but it is a close enough approximation -- it is near
> > the point where we could simply agree to use it and stop worrying
> > about styling issues. However, that would mean everyone needs to have
> > a recent clang-format available, which I think is the biggest obstacle
> > at the moment.
> 
> I don't think that's close to true yet for clang-format.
> 
> For instance: clang-format does not do anything with
> missing braces, or coalescing multi-part strings,
> or any number of other nominal coding style defects
> like all the for_each macros, aligning or not aligning
> columnar contents appropriately, etc...
> 
> clang-format as yet has no taste.
> 
> I believe it'll take a lot of work to improve it to a point
> where its formatting is acceptable and appropriate.
> 
> An AI rather than a table based system like clang-format is
> more likely to be a real solution, but training that AI
> isn't a thing that I want to do.

and an example very poor conversion from that same thread:

        unsigned int key, newkey;
        int i;
 
-       rc = sscanf(buf, "%"__stringify(SEC_CMD_SIZE)"s"
-                   " %"__stringify(KEY_ID_SIZE)"s"
-                   " %"__stringify(KEY_ID_SIZE)"s",
-                   cmd, keystr, nkeystr);
+       rc = sscanf(
+               buf,
+               "%" __stringify(
+                       SEC_CMD_SIZE) "s"
+                                     " %" __stringify(
+                                             KEY_ID_SIZE) "s"
+                                                          " %" __stringify(
+                                                                  KEY_ID_SIZE) "s",
+               cmd, keystr, nkeystr);



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  0:56   ` Joe Perches
@ 2020-11-04  3:10     ` Miguel Ojeda
  2020-11-04  3:15       ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-04  3:10 UTC (permalink / raw)
  To: Joe Perches; +Cc: clang-built-linux, LKML

On Wed, Nov 4, 2020 at 1:56 AM Joe Perches <joe@perches.com> wrote:
>
> Do remember that this patch is for the current kernel and
> not any old version that someone might be compiling.
>
> The current kernel _requires_ clang 10.0+ and that would
> obviously provide clang-format 10+ as well.

You can use clang-format without having ever built a kernel with Clang.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  3:10     ` Miguel Ojeda
@ 2020-11-04  3:15       ` Joe Perches
  2020-11-04  3:57         ` Miguel Ojeda
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2020-11-04  3:15 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: clang-built-linux, LKML

On Wed, 2020-11-04 at 04:10 +0100, Miguel Ojeda wrote:
> On Wed, Nov 4, 2020 at 1:56 AM Joe Perches <joe@perches.com> wrote:
> > 
> > Do remember that this patch is for the current kernel and
> > not any old version that someone might be compiling.
> > 
> > The current kernel _requires_ clang 10.0+ and that would
> > obviously provide clang-format 10+ as well.
> 
> You can use clang-format without having ever built a kernel with Clang.

No one ever will use clang-format on the current kernel sources
without having a recent version of clang and clang-format.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  1:08   ` Nick Desaulniers
  2020-11-04  1:31     ` Joe Perches
@ 2020-11-04  3:40     ` Miguel Ojeda
  2020-11-05  0:33       ` Nick Desaulniers
  1 sibling, 1 reply; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-04  3:40 UTC (permalink / raw)
  To: Nick Desaulniers; +Cc: Joe Perches, clang-built-linux, LKML

On Wed, Nov 4, 2020 at 2:08 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Miguel,
> Really? :P I'd bet if you picked up this patch no one would notice.
>
> I recommend a simpler approach to multiple version support, which is
> just matching the one version recommended for the rest of LLVM tools.
> Sure, technically you can use older tools, but do so at your own peril
> and don't complain to us if it doesn't work.  Otherwise trying to

Originally, the .clang-format file was made to work with old versions
in order to make it easy for people to use (just install it from your
distro etc.). One of the reasons for that was to help adoption, as
well as because clang-format gives a hard error on encountering an
unknown option :-(

I am not opposed to changing those requirements now and say it is part
of the LLVM toolchain (even if it is independent from building), but
somebody might be annoyed.

> explain different versions and even for different directories gets way
> too complex for anyone to take seriously.

That is just an escape hatch for developers that really need the
latest formatting options (e.g. to minimize the exceptions in fully
formatted files) or temporarily deal with some bits of kernel code
with a different style.

I definitely wouldn't want people adding their own .clang-format files
without good reason...

> It's not like we backport raising the minimum version.

That is a good point. In fact, we can just do it very early in the
cycle and wait to see who complains. If there are too many complaints,
we can always revert it.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  3:15       ` Joe Perches
@ 2020-11-04  3:57         ` Miguel Ojeda
  2020-11-04  4:16           ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-04  3:57 UTC (permalink / raw)
  To: Joe Perches; +Cc: clang-built-linux, LKML

On Wed, Nov 4, 2020 at 4:15 AM Joe Perches <joe@perches.com> wrote:
>
> No one ever will use clang-format on the current kernel sources
> without having a recent version of clang and clang-format.

Why? Many distros come with clang-format pre-packaged, and in fact the
original patch (that you commented on) argued for the >= 4 requirement
that way.

I am not saying we cannot change it, in fact I'd prefer if we do it
(see my answer to Nick); but I don't understand your basis to claim
nobody is installing their distro clang-format package.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  3:57         ` Miguel Ojeda
@ 2020-11-04  4:16           ` Joe Perches
  2020-11-04  5:24             ` Miguel Ojeda
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2020-11-04  4:16 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: clang-built-linux, LKML

On Wed, 2020-11-04 at 04:57 +0100, Miguel Ojeda wrote:
> On Wed, Nov 4, 2020 at 4:15 AM Joe Perches <joe@perches.com> wrote:
> > 
> > No one ever will use clang-format on the current kernel sources
> > without having a recent version of clang and clang-format.
> 
> Why? Many distros come with clang-format pre-packaged, and in fact the
> original patch (that you commented on) argued for the >= 4 requirement
> that way.

The current kernel is v5.10 which requires clang 10.0 or higher.
This patch is for the current kernel.

This patch is not to be applied or backported to old kernels so no
person is going to use this patch on any old or backported kernel.

If a person is going to use clang-format on the current kernel sources
unless they are developing for the current kernel.

They are going to have to be using clang 10.0 or higher and therefore
also will have and be using clang-format 10.0 or higher.

Take it or not, apply it or not.  I don't use clang-format and unless
there are improvements to it, I imagine I'll continue to use emacs
indent-region and a few other reformatting tools instead.

cheers, Joe


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  4:16           ` Joe Perches
@ 2020-11-04  5:24             ` Miguel Ojeda
  0 siblings, 0 replies; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-04  5:24 UTC (permalink / raw)
  To: Joe Perches; +Cc: clang-built-linux, LKML

Hi Joe,

First of all, thanks for taking the time to write your reasoning.

On Wed, Nov 4, 2020 at 5:17 AM Joe Perches <joe@perches.com> wrote:
>
> The current kernel is v5.10 which requires clang 10.0 or higher.

For building, yes.

> This patch is not to be applied or backported to old kernels so no
> person is going to use this patch on any old or backported kernel.

Agreed (see my answer to Nick).

> If a person is going to use clang-format on the current kernel sources
> unless they are developing for the current kernel.
>
> They are going to have to be using clang 10.0 or higher and therefore
> also will have and be using clang-format 10.0 or higher.

No, they might be using GCC as usual and installed clang-format from
their distro. In fact, I'd expect most developers accustomed to GCC to
try it out that way, and also most of them to install compilers from
their distro, not from the webpage, unless they need a newer version
for some reason (e.g. new warnings, new debugging features in the
kernel, etc.).

In principle, clang-format (as a tool) is not related to building the
kernel. We may call it "x-format" and think about it as a statically
linked binary. What I am saying is that aligning clang-format to LLVM
(now that LLVM has a minimum supported version) is not a necessity.

We can still do it, of course, since there are new features for
everyone and anybody that complains can install a newer version from
the webpage. But there is nothing that forces us to require it. It is
a decision that we balance w.r.t. new features. To put it concretely:
if there were 0 new features or big fixes in clang-format 10 compared
to 4, there would be no reason whatsoever to require users to download
a new version.

On the other side of the spectrum, some projects require a concrete
version (not just a minimum), because they automatically format their
entire codebase and want to avoid differences in output between
clang-format versions. But we are far from automatically formatting
the entire codebase.

> Take it or not, apply it or not.  I don't use clang-format and unless
> there are improvements to it, I imagine I'll continue to use emacs
> indent-region and a few other reformatting tools instead.

Again, I am not opposed to the change. In fact, I am eager to improve
the output of clang-format so that more people get engaged with it. I
was just surprised you asserted so strongly that nobody is using
clang-format from their distro.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  3:40     ` Miguel Ojeda
@ 2020-11-05  0:33       ` Nick Desaulniers
  2020-11-05  5:10         ` Miguel Ojeda
  0 siblings, 1 reply; 17+ messages in thread
From: Nick Desaulniers @ 2020-11-05  0:33 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Joe Perches, clang-built-linux, LKML

On Tue, Nov 3, 2020 at 7:40 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> distro etc.). One of the reasons for that was to help adoption, as
> well as because clang-format gives a hard error on encountering an
> unknown option :-(

Yeah, that's annoying. Why don't you send me a patch that downgrades
it from hard error to polite warning (in case someone made a typo),
and I'll review it?
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-04  1:31     ` Joe Perches
@ 2020-11-05  0:35       ` Nick Desaulniers
  2020-11-05  6:44         ` Miguel Ojeda
  0 siblings, 1 reply; 17+ messages in thread
From: Nick Desaulniers @ 2020-11-05  0:35 UTC (permalink / raw)
  To: Joe Perches; +Cc: Miguel Ojeda, clang-built-linux, LKML

On Tue, Nov 3, 2020 at 5:31 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-11-03 at 17:08 -0800, Nick Desaulniers wrote:
> > On Tue, Nov 3, 2020 at 1:33 PM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > > On Tue, Nov 3, 2020 at 7:29 PM Joe Perches <joe@perches.com> wrote:
> > > >
> > > > Now that the clang minimum supported version is > 10.0, enable the
> > > > commented out conditional reformatting key:value lines in the file.
> > > >
> > > > Signed-off-by: Joe Perches <joe@perches.com>
> > > > ---
> > > >
> > > > Hey Miguel.
> > > >
> > > > I don't use this, but on its face it seems a reasonable change
> > > > if the commented out key:value lines are correct.
> >
> > Joe,
> > what would it take to get you to use clang-format, or at least try it?
> >  Beers?  Bribes? Dirty deeds, done dirt cheap?
>
> Hey Nick.
>
> Paint my house? ;)

I'll help you paint your bikeshed. Oh, but what color?  I see a red
door, and I want it painted black.

Sounds to me like Miguel could win over a critic by addressing some of
your (quite valid) concerns. ;)
--
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-05  0:33       ` Nick Desaulniers
@ 2020-11-05  5:10         ` Miguel Ojeda
  0 siblings, 0 replies; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-05  5:10 UTC (permalink / raw)
  To: Nick Desaulniers; +Cc: Joe Perches, clang-built-linux, LKML

On Thu, Nov 5, 2020 at 1:33 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> Yeah, that's annoying. Why don't you send me a patch that downgrades
> it from hard error to polite warning (in case someone made a typo),
> and I'll review it?

Sure!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-05  0:35       ` Nick Desaulniers
@ 2020-11-05  6:44         ` Miguel Ojeda
  2020-11-05  7:08           ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Miguel Ojeda @ 2020-11-05  6:44 UTC (permalink / raw)
  To: Nick Desaulniers; +Cc: Joe Perches, clang-built-linux, LKML

On Thu, Nov 5, 2020 at 1:35 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> I'll help you paint your bikeshed. Oh, but what color?  I see a red
> door, and I want it painted black.
>
> Sounds to me like Miguel could win over a critic by addressing some of
> your (quite valid) concerns. ;)

I am happy to take the patch, but we need to at least enable other
features that were added meanwhile -- that is the advantage of
increasing the minimum!

If someone wants to take a look (wink, wink -- Joe, you are very
experienced with all the different styles used around the kernel and
would be great to have you on board with clang-format), they can use
the following list for starters.

There are a few important new features:
  - AlignConsecutiveMacros is probably one of the biggest one for the
kernel that we were missing so far.
  - IndentPPDirectives and AlignEscapedNewlines would be very good to
use too -- but the kernel style is not consistent AFAIK, so we would
need to decide.

Then there are a few others that pertain to us too:
  - SpaceBeforeSquareBrackets
  - SpacesInConditionalStatement
  - SpaceAfterLogicalNot
  - SpaceInEmptyBlock
  - IndentGotoLabels

Others are also worth checking to see if we can take advantage of them:
  - IncludeBlocks (and configuring IncludeCategories etc.)
  - StatementMacros

Then there are others that are not related to us, but to be consistent
we would explicitly set them in the file. Finally, for extra points,
we could already document the new ones in LLVM 11 if any, for the
future, but that is optional.

If no one is up for the task, I will eventually do it... :-)

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC PATCH] .clang-format: Remove conditional comments
  2020-11-05  6:44         ` Miguel Ojeda
@ 2020-11-05  7:08           ` Joe Perches
  0 siblings, 0 replies; 17+ messages in thread
From: Joe Perches @ 2020-11-05  7:08 UTC (permalink / raw)
  To: Miguel Ojeda, Nick Desaulniers; +Cc: clang-built-linux, LKML

On Thu, 2020-11-05 at 07:44 +0100, Miguel Ojeda wrote:
> There are a few important new features:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html

>   - AlignConsecutiveMacros is probably one of the biggest one for the
> kernel that we were missing so far.

There's no control as to effective column nor sensible mechanism to
avoid extremely long indents with a single exceptional entry.

>   - IndentPPDirectives and

Some yes, mostly no.

AlignEscapedNewlines:

Generally the kernel uses column 72 but there's not real consistency.
clang-format doesn't have that option as far as I can tell.

> Then there are a few others that pertain to us too:
>   - SpaceBeforeSquareBrackets

no

>   - SpacesInConditionalStatement

no
 
>   - SpaceAfterLogicalNot

no

>   - SpaceInEmptyBlock

no

>   - IndentGotoLabels

no

> 
> Others are also worth checking to see if we can take advantage of them:
>   - IncludeBlocks (and configuring IncludeCategories etc.)

Might be worthwhile.  It's different by maintainer preference though.
Reverse Xmas tree is somewhat common in networking, (which I think is
silly, but DaveM likes it).  Some like alphabetic ordering, others
by order of include.

>   - StatementMacros

Kernel is not c++ so this is irrelevant for gcc macro statement expressions.

> Then there are others that are not related to us, but to be consistent
> we would explicitly set them in the file. Finally, for extra points,
> we could already document the new ones in LLVM 11 if any, for the
> future, but that is optional.
> 
> If no one is up for the task, I will eventually do it... :-)

Enjoy...


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-11-05  7:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 18:29 [RFC PATCH] .clang-format: Remove conditional comments Joe Perches
2020-11-03 21:33 ` Miguel Ojeda
2020-11-04  0:56   ` Joe Perches
2020-11-04  3:10     ` Miguel Ojeda
2020-11-04  3:15       ` Joe Perches
2020-11-04  3:57         ` Miguel Ojeda
2020-11-04  4:16           ` Joe Perches
2020-11-04  5:24             ` Miguel Ojeda
2020-11-04  1:08   ` Nick Desaulniers
2020-11-04  1:31     ` Joe Perches
2020-11-05  0:35       ` Nick Desaulniers
2020-11-05  6:44         ` Miguel Ojeda
2020-11-05  7:08           ` Joe Perches
2020-11-04  3:40     ` Miguel Ojeda
2020-11-05  0:33       ` Nick Desaulniers
2020-11-05  5:10         ` Miguel Ojeda
2020-11-04  1:11 ` Nick Desaulniers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).