Re: [RFC PATCH 5/5] xen: Add clang-format configuration

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 9 Aug 2023, at 16:48, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 28.07.2023 10:11, Luca Fancellu wrote:
>> --- /dev/null
>> +++ b/xen/.clang-format
>> @@ -0,0 +1,693 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# clang-format configuration file. Intended for clang-format >= 15.
>> +#
>> +# For more information, see:
>> +#
>> +#   Documentation/process/clang-format.rst
>> +#   https://clang.llvm.org/docs/ClangFormat.html
>> +#   https://clang.llvm.org/docs/ClangFormatStyleOptions.html
>> +#
>> +---
>> +
>> +# [not specified]
>> +# Align function parameter that goes into a new line, under the open bracket
>> +# (supported in clang-format 3.8)
>> +AlignAfterOpenBracket: Align
> 
> I'm not convinced this rule (assuming I'm getting it right) is
> suitable in all cases, especially for functions with long names or
> very many parameters.

Not sure I understand, I think this is the current behaviour in the codebase 
now.

> 
>> +# [not specified]
>> +# Align array of struct's elements by column and justify
>> +# struct test demo[] =
>> +# {
>> +#     {56, 23,    "hello"},
>> +#     {-1, 93463, "world"},
>> +#     {7,  5,     "!!"   }
>> +# };
> 
> I'm pretty sure we want to have blanks immediately inside the braces.

ok

> 
>> +# (supported in clang-format 13)
>> +AlignArrayOfStructures: Left
>> +
>> +# [not specified]
>> +# Align consecutive assignments (supported in clang-format 3.8)
>> +AlignConsecutiveAssignments:
>> +  Enabled: true
>> +  AcrossEmptyLines: true
>> +  AcrossComments: false
> 
> This is something we want to permit, but not demand, I think. I'm also
> unconvinced we want it across empty lines.

This was pointed out by Alejandro and Stefano, we can disable this by
passing 'None'

> 
>> +# [not specified]
>> +# Do not align consecutive bit fields (supported in clang-format 11)
>> +AlignConsecutiveBitFields: None
>> +
>> +# [not specified]
>> +# Do not align values of consecutive declarations
>> +# (supported in clang-format 3.8)
>> +AlignConsecutiveDeclarations: None
>> +
>> +# [not specified]
>> +# Align values of consecutive macros (supported in clang-format 9)
>> +AlignConsecutiveMacros:
>> +  Enabled: true
>> +  AcrossEmptyLines: true
>> +  AcrossComments: true
> 
> This also looks to aggressive to me.

It can be, but it can lead to a nice format. Anyway can be customised.

> 
> And I guess I'll stop here. What is the plan wrt discussing which
> aspects we want to require and which we want to permit but not
> require? Or is there alternatively a way to leave unconfigured
> (and hence unaltered) anything that's not already spelled out in
> ./CODING_STYLE?

I think I did create a configuration specifying only what was spelled out
in CODING_STYLE, but it was not really successful and led to many changes,
more than now.

I think we need to collect maintainers view on every [not specified] 
configurable
and see which one are agreed by the majority, and which one are more 
controversial.

I’ve pushed this serie also to make the community able to see the output and 
raise
issues or behaviour that are not right, to see if we can do something or if 
there is no
agreement and the tool would be put on hold.

Anyway I’m aware that we are going into a period that will be pretty busy 
between
annual leaves and the release, so I’m ok to ping this discussion after the 
release.

Cheers,
Luca

> 
> Jan