list iterator spacing: clang-format vs checkpatch

list iterator spacing: clang-format vs checkpatch

[Jason A. Donenfeld]

Hi Joe, Miguel, others,

The shiny new .clang-format file lists a number of nice iterators in
the ForEachMacros category, the consequence being that there is a
space between the iterator name and the opening parenthesis. This
strikes me as the right thing to do.

However, checkpatch.pl complains about it:

WARNING: space prohibited between function name and open parenthesis '('
#65: FILE: ratelimiter.c:65:
+               hlist_for_each_entry_safe (entry, temp, &table_v4[i], hash) {

It would seem that .clang-format is right and checkpatch.pl is wrong?

Thanks,
Jason

Re: list iterator spacing: clang-format vs checkpatch

[Miguel Ojeda]

Hi Jason,

On Mon, Oct 8, 2018 at 4:01 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Joe, Miguel, others,
>
> The shiny new .clang-format file lists a number of nice iterators in
> the ForEachMacros category, the consequence being that there is a
> space between the iterator name and the opening parenthesis. This
> strikes me as the right thing to do.
>
> However, checkpatch.pl complains about it:
>
> WARNING: space prohibited between function name and open parenthesis '('
> #65: FILE: ratelimiter.c:65:
> +               hlist_for_each_entry_safe (entry, temp, &table_v4[i], hash) {
>
> It would seem that .clang-format is right and checkpatch.pl is wrong?

Checking quickly, it would seem most of the kernel does not put a
space there (a minority does), e.g.:

  git grep 'list_for_each[a-zA-Z0-9_]* (' | wc -l # 67
  git grep 'list_for_each[a-zA-Z0-9_]*(' | wc -l # 13892

So in that sense, checkpatch.pl is right (even if it is not a function call).

Now, I put the list there because otherwise clang-format would put
braces in the next line, which looks even worse.

I am not sure if there is a way to make clang-format do what we need:
  * SpaceBeforeParens does not have an option to distinguish normal
loops from macro ones.
  * SpaceBeforeRangeBasedForLoopColon does not do it (which makes
sense, but it was a nice try :-)

We would probably need to implement SpaceBeforeForEachMacros (or a new
option for SpaceBeforeParens). I haven't had the time to look into
adding the missing support for the few things that the kernel style
requires, sadly, but it is in my TODO list.

Cheers,
Miguel

Re: list iterator spacing: clang-format vs checkpatch

[Joe Perches]

On Mon, 2018-10-08 at 09:31 +0200, Miguel Ojeda wrote:
> On Mon, Oct 8, 2018 at 4:01 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > The shiny new .clang-format file lists a number of nice iterators in
> > the ForEachMacros category, the consequence being that there is a
> > space between the iterator name and the opening parenthesis. This
> > strikes me as the right thing to do.
 
It does not strike me as the right thing to do.

Keeping an exhaustive list current is a continuing
burden and the list generally goes stale over time.

> > However, checkpatch.pl complains about it:
> > 
> > WARNING: space prohibited between function name and open parenthesis '('
> > #65: FILE: ratelimiter.c:65:
> > +               hlist_for_each_entry_safe (entry, temp, &table_v4[i], hash) {
> > 
> > It would seem that .clang-format is right and checkpatch.pl is wrong?
> 
> Checking quickly, it would seem most of the kernel does not put a
> space there (a minority does), e.g.:
> 
>   git grep 'list_for_each[a-zA-Z0-9_]* (' | wc -l # 67
>   git grep 'list_for_each[a-zA-Z0-9_]*(' | wc -l # 13892
> 
> So in that sense, checkpatch.pl is right (even if it is not a function call).

As a general rule, I believe any dominant coding style
is correct. These things are just a convention.

.clang-format is a work in progress and should be updated
where possible to reflect the kernel dominant styles.

> We would probably need to implement SpaceBeforeForEachMacros (or a new
> option for SpaceBeforeParens). I haven't had the time to look into
> adding the missing support for the few things that the kernel style
> requires, sadly, but it is in my TODO list.

Good luck and as you are the .clang_format maintainer,
I hope you find the time.

Re: list iterator spacing: clang-format vs checkpatch

[Miguel Ojeda]

On Mon, Oct 8, 2018 at 5:40 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2018-10-08 at 09:31 +0200, Miguel Ojeda wrote:
> > On Mon, Oct 8, 2018 at 4:01 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > The shiny new .clang-format file lists a number of nice iterators in
> > > the ForEachMacros category, the consequence being that there is a
> > > space between the iterator name and the opening parenthesis. This
> > > strikes me as the right thing to do.
>
> It does not strike me as the right thing to do.
>
> Keeping an exhaustive list current is a continuing
> burden and the list generally goes stale over time.

Indeed, it is not nice. It would be best if clang-format had a special
case for macros before a block (i.e. assuming those are meant to be
"for loop macros"). Alas...

>
> > > However, checkpatch.pl complains about it:
> > >
> > > WARNING: space prohibited between function name and open parenthesis '('
> > > #65: FILE: ratelimiter.c:65:
> > > +               hlist_for_each_entry_safe (entry, temp, &table_v4[i], hash) {
> > >
> > > It would seem that .clang-format is right and checkpatch.pl is wrong?
> >
> > Checking quickly, it would seem most of the kernel does not put a
> > space there (a minority does), e.g.:
> >
> >   git grep 'list_for_each[a-zA-Z0-9_]* (' | wc -l # 67
> >   git grep 'list_for_each[a-zA-Z0-9_]*(' | wc -l # 13892
> >
> > So in that sense, checkpatch.pl is right (even if it is not a function call).
>
> As a general rule, I believe any dominant coding style
> is correct. These things are just a convention.
>
> .clang-format is a work in progress and should be updated
> where possible to reflect the kernel dominant styles.

Ideally, in a handful of years we would have an almost perfect mapping
(and/or agree to dismiss the old style that cannot be easily emulated)
and simply force to pass all code through clang-format in a
(server-side) git hook. One can dream... :-)

>
> > We would probably need to implement SpaceBeforeForEachMacros (or a new
> > option for SpaceBeforeParens). I haven't had the time to look into
> > adding the missing support for the few things that the kernel style
> > requires, sadly, but it is in my TODO list.
>
> Good luck and as you are the .clang_format maintainer,
> I hope you find the time.
>

Thanks a lot! I will try for sure at some point, since coding style
stuff takes a looooot of time from everyone.

Cheers,
Miguel

Re: list iterator spacing: clang-format vs checkpatch

[Jason A. Donenfeld]

Thanks for the clarification Joe. I'll adjust my codebase to roll with
checkpatch's conventions.

Re: list iterator spacing: clang-format vs checkpatch

[Jason A. Donenfeld]

Hey again,

Four years later I went through basically the same motions: "oh hey I
should clean this up", "I'll start with clang format", "oh cool it
adds spaces before the iterator paren so it looks like a normal for
loop to me", "that seems so reasonable; I love clang format", "oh no
checkpatch.pl complains; I hope it's wrong", "I wonder if anybody has
thought about this before", "oh, look, I asked about this already in
2018."

So, here we are again. I'm wondering:
- Can we switch to spaces before iterator parens?
- If not, is clang-format ever going to be fixed to quit adding them?

Regards,
Jason

Re: list iterator spacing: clang-format vs checkpatch

[Joe Perches]

On Mon, 2022-01-17 at 13:47 +0100, Jason A. Donenfeld wrote:
> Hey again,

Rehi.

> Four years later I went through basically the same motions: "oh hey I
> should clean this up", "I'll start with clang format", "oh cool it
> adds spaces before the iterator paren so it looks like a normal for
> loop to me", "that seems so reasonable; I love clang format", "oh no
> checkpatch.pl complains; I hope it's wrong", "I wonder if anybody has
> thought about this before", "oh, look, I asked about this already in
> 2018."

Original thread:

https://lore.kernel.org/lkml/CAHmME9ofzanQTBD_WYBRW49d+gM77rCdh8Utdk4+PM9n_bmKwA@mail.gmail.com/

> So, here we are again. I'm wondering:
> - Can we switch to spaces before iterator parens?

Still doubtful because the kernel sources has ~150:1 preference
for no space, and it's still just a whitespace convention...

$ git grep -P '\b\w*for_each\w*\(' | wc -l
31920
$ git grep -P '\b\w*for_each\w*\s+\(' | wc -l
196

> - If not, is clang-format ever going to be fixed to quit adding them?

Doubtful as it's likely the .clang-format for_each list is
just out of date for your particular for_each type use and
the scripted bit that it uses to create them hasn't be
updated in awhile.  Also that scripted bit only works on files
in include/ and not anything locally defined.

in .clang-format:

# Taken from:
#   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ \
#   | sed "s,^#define \([^[:space:]]*for_each[^[:space:]]*\)(.*$,  - '\1'," \
#   | sort | uniq

commit 4792f9dd12936ec35deced665ae3a4ca8fe98729
Author: Miguel Ojeda <ojeda@kernel.org>
Date:   Wed May 12 23:32:39 2021 +0200

    clang-format: Update with the latest for_each macro list
    
    Re-run the shell fragment that generated the original list.
    
    Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

checkpatch basically just looks for any use of 'for_each'

(?:[a-z_]+|)for_each[a-z_]+)

So it has false positives for some functions and not macros.

Re: list iterator spacing: clang-format vs checkpatch

[Miguel Ojeda]

Hi Joe,

On Mon, Jan 17, 2022 at 7:05 PM Joe Perches <joe@perches.com> wrote:
>
> Doubtful as it's likely the .clang-format for_each list is
> just out of date for your particular for_each type use and
> the scripted bit that it uses to create them hasn't be

I will send an update.

> updated in awhile.  Also that scripted bit only works on files
> in include/ and not anything locally defined.
>
> [...]
>
> So it has false positives for some functions and not macros.

Yeah, for `clang-format` I tried to be conservative having only
`include/`, but we could change that.

Cheers,
Miguel

Re: list iterator spacing: clang-format vs checkpatch

[Joe Perches]

On Mon, 2022-01-17 at 22:41 +0100, Miguel Ojeda wrote:
> Yeah, for `clang-format` I tried to be conservative having only
> `include/`, but we could change that.

There are more #defines outside of include/ than inside:

$ git grep -P '^\s*#\s*define\s+\w*for_each\w*\(' -- '*.[ch]' | \
  grep -P -v '^(?:include|tools)/' | wc -l
613

$ git grep -P '^\s*#\s*define\s+\w*for_each\w*\(' -- 'include/*.[ch]' | \
  wc -l
469

RE: list iterator spacing: clang-format vs checkpatch

[David Laight]

From: Joe Perches
> Sent: 17 January 2022 18:05
> 
> On Mon, 2022-01-17 at 13:47 +0100, Jason A. Donenfeld wrote:
> > Hey again,
> 
> Rehi.
> 
> > Four years later I went through basically the same motions: "oh hey I
> > should clean this up", "I'll start with clang format", "oh cool it
> > adds spaces before the iterator paren so it looks like a normal for
> > loop to me", "that seems so reasonable; I love clang format", "oh no
> > checkpatch.pl complains; I hope it's wrong", "I wonder if anybody has
> > thought about this before", "oh, look, I asked about this already in
> > 2018."

Personally I think it should look like a #define expansion, not
part of the language.

I did notice it in the recent patch - and though it looked wrong.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: list iterator spacing: clang-format vs checkpatch

[Miguel Ojeda]

Hi Jason,

On Mon, Jan 17, 2022 at 1:47 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> - If not, is clang-format ever going to be fixed to quit adding them?

`ControlStatementsExceptForEachMacros` was added in LLVM 11 (and
`SpaceBeforeParensOptions` in LLVM 14, which gives even more
fine-grained control).

So it is coming -- the question is whether we wait a bit until LLVM 11
is the minimum supported version (nowadays LLVM 10) or we are willing
to require LLVM 11.

Cheers,
Miguel

Re: list iterator spacing: clang-format vs checkpatch

[Miguel Ojeda]

On Mon, Jan 17, 2022 at 10:41 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> So it is coming -- the question is whether we wait a bit until LLVM 11
> is the minimum supported version (nowadays LLVM 10) or we are willing
> to require LLVM 11.

LLVM 11 is now the minimum with commit df05c0e9496c ("Documentation:
Raise the minimum supported version of LLVM to 11.0.0"), so I will
give this a go.

Cheers,
Miguel

Re: list iterator spacing: clang-format vs checkpatch

[Brian Norris]

Hi Miguel,

On Sat, Jan 22, 2022 at 02:16:14PM +0100, Miguel Ojeda wrote:
> On Mon, Jan 17, 2022 at 10:41 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > So it is coming -- the question is whether we wait a bit until LLVM 11
> > is the minimum supported version (nowadays LLVM 10) or we are willing
> > to require LLVM 11.
> 
> LLVM 11 is now the minimum with commit df05c0e9496c ("Documentation:
> Raise the minimum supported version of LLVM to 11.0.0"), so I will
> give this a go.

How's it going? Are you ready to apply this patch?

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 .clang-format | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.clang-format b/.clang-format
index fa959436bcfd..63f0127a409d 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 >= 11.
 #
 # For more information, see:
 #
@@ -545,7 +545,7 @@ SpaceAfterTemplateKeyword: true
 SpaceBeforeAssignmentOperators: true
 #SpaceBeforeCtorInitializerColon: true # Unknown to clang-format-5.0
 #SpaceBeforeInheritanceColon: true # Unknown to clang-format-5.0
-SpaceBeforeParens: ControlStatements
+SpaceBeforeParens: ControlStatementsExceptForEachMacros
 #SpaceBeforeRangeBasedForLoopColon: true # Unknown to clang-format-5.0
 SpaceInEmptyParentheses: false
 SpacesBeforeTrailingComments: 1
-- 
2.36.0

Re: list iterator spacing: clang-format vs checkpatch

[Miguel Ojeda]

Hi Brian,

On Fri, Apr 22, 2022 at 1:53 AM Brian Norris <briannorris@chromium.org> wrote:
>
> How's it going? Are you ready to apply this patch?

My intention is to do a wider review of the file (e.g. remove all the
"Unknown to"s, check if any option should be changed, etc.). Let me do
it this kernel cycle and apply your diff.

Cheers,
Miguel