Clang-format configuration discussion - pt 1

Clang-format configuration discussion - pt 1

[Luca Fancellu]

Hi all,

Let’s kick off the discussion about clang-format configuration, with this part 1 I would like to discuss some configurable
that I feel are not controversial.

You can find the serie introducing clang-format here:
https://patchwork.kernel.org/project/xen-devel/cover/20231031132304.2573924-1-luca.fancellu@arm.com/
and there is also a patch linked to my gitlab account where you can find the output for the hypervisor code.

For a full list of configurables and to find the possible values for them, please refer to this page:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

--------------------------------------------------------------------------------------------------------------------------------------------------------------

ColumnLimit: 80

IndentWidth: 4

TabWidth: 4

UseTab: Never

---
Our coding style states it explicitly:
[...]
Indenting uses spaces, not tabs - in contrast to Linux. An indent
level consists of four spaces. Code within blocks is indented by one
extra indent level.
[...]

--------------------------------------------------------------------------------------------------------------------------------------------------------------

Language: Cpp

---
As the clang-format documentation says: Should be used for C, C++.

--------------------------------------------------------------------------------------------------------------------------------------------------------------

Standard: C++03

---
From the documentation: Parse and format C++ constructs compatible with this standard.

This value is used also in Linux.

--------------------------------------------------------------------------------------------------------------------------------------------------------------

AttributeMacros:
  - '__init'
  - '__exit'
  - '__initdata'
  - '__initconst'
  - '__initconstrel'
  - '__initdata_cf_clobber'
  - '__initconst_cf_clobber'
  - '__hwdom_init'
  - '__hwdom_initdata'
  - '__maybe_unused'
  - '__packed'
  - '__stdcall'
  - '__vfp_aligned'
  - '__alt_call_maybe_initdata'
  - '__cacheline_aligned'
  - '__ro_after_init'
  - 'always_inline'
  - 'noinline'
  - 'noreturn'
  - '__weak'
  - '__inline__'
  - '__attribute_const__'
  - '__transparent__'
  - '__used'
  - '__must_check'
  - '__kprobes'

---
A vector of strings that should be interpreted as attributes/qualifiers instead of identifiers.
I’ve tried to list all the attributes I’ve found.

--------------------------------------------------------------------------------------------------------------------------------------------------------------

MacroBlockBegin: '^PLATFORM_START|^DT_DEVICE_START|^ACPI_DEVICE_START'

MacroBlockEnd: '^PLATFORM_END|^DT_DEVICE_END|^ACPI_DEVICE_END’

---
Regular expressions that matches begin and end of a block.
e.g.

PLATFORM_START(rcar2, "Renesas R-Car Gen2")
.compatible = rcar2_dt_compat,
.cpu_up = cpu_up_send_sgi,
.smp_init = rcar2_smp_init,
PLATFORM_END

--------------------------------------------------------------------------------------------------------------------------------------------------------------

StatementMacros:
  - 'PROGRESS'
  - 'PROGRESS_VCPU'
  - 'bitop'
  - 'guest_bitop'
  - 'testop'
  - 'guest_testop'
  - 'DEFINE_XEN_GUEST_HANDLE'
  - '__DEFINE_XEN_GUEST_HANDLE'
  - '___DEFINE_XEN_GUEST_HANDLE'
  - 'presmp_initcall'
  - '__initcall'
  - '__exitcall'

---
A vector of macros that should be interpreted as complete statements.
Typical macros are expressions, and require a semi-colon to be added; sometimes this is not the case, and this allows
to make clang-format aware of such cases.

While I was writing this, I’ve found that from ‘DEFINE_XEN_GUEST_HANDLE’ until the end of the list, probably I
shouldn’t list these entries because all of them end with semi-colon.

--------------------------------------------------------------------------------------------------------------------------------------------------------------

TypenameMacros:
  - 'XEN_GUEST_HANDLE'
  - 'XEN_GUEST_HANDLE_64'
  - 'XEN_GUEST_HANDLE_PARAM'
  - 'ELF_HANDLE_DECL'

---
A vector of macros that should be interpreted as type declarations instead of as function calls.
These are expected to be macros of the form:
STACK_OF(...)

--------------------------------------------------------------------------------------------------------------------------------------------------------------

WhitespaceSensitiveMacros:
  - '__stringify'

---
A vector of macros which are whitespace-sensitive and should not be touched.

--------------------------------------------------------------------------------------------------------------------------------------------------------------

Ok this is it for now, let me know your thoughts about them, ideally if I don’t get any reply in two weeks (22nd of November),
I will consider that we have an agreement on these configuration.

Cheers,
Luca

Re: Clang-format configuration discussion - pt 1

[Jan Beulich]

On 08.11.2023 10:53, Luca Fancellu wrote:
--------------------------------------------------------------------------------------------------------------------------------------------------------------
> 
> Standard: C++03
> 
> ---
> From the documentation: Parse and format C++ constructs compatible with this standard.

Since I continue to be puzzled - iirc you said this is because of lack
of availability of "C99" as a value here. What's entirely unclear to
me is: How does this matter to a tool checking coding style (which is
largely about formatting, not any lexical or syntactical aspects)?

> This value is used also in Linux.

Considering how different the two styles are, I don't think this is
overly relevant.

--------------------------------------------------------------------------------------------------------------------------------------------------------------
> 
> AttributeMacros:
>   - '__init'
>   - '__exit'
>   - '__initdata'
>   - '__initconst'
>   - '__initconstrel'
>   - '__initdata_cf_clobber'
>   - '__initconst_cf_clobber'
>   - '__hwdom_init'
>   - '__hwdom_initdata'
>   - '__maybe_unused'
>   - '__packed'
>   - '__stdcall'
>   - '__vfp_aligned'
>   - '__alt_call_maybe_initdata'
>   - '__cacheline_aligned'
>   - '__ro_after_init'
>   - 'always_inline'
>   - 'noinline'
>   - 'noreturn'
>   - '__weak'
>   - '__inline__'
>   - '__attribute_const__'
>   - '__transparent__'
>   - '__used'
>   - '__must_check'
>   - '__kprobes'
> 
> ---
> A vector of strings that should be interpreted as attributes/qualifiers instead of identifiers.
> I’ve tried to list all the attributes I’ve found.

Like above, the significance of having this list and needing to keep it
up-to-date is unclear to me. I guess the same also applies to a few
further settings down from here.

--------------------------------------------------------------------------------------------------------------------------------------------------------------
> 
> StatementMacros:
>   - 'PROGRESS'
>   - 'PROGRESS_VCPU'
>   - 'bitop'
>   - 'guest_bitop'
>   - 'testop'
>   - 'guest_testop'
>   - 'DEFINE_XEN_GUEST_HANDLE'
>   - '__DEFINE_XEN_GUEST_HANDLE'
>   - '___DEFINE_XEN_GUEST_HANDLE'
>   - 'presmp_initcall'
>   - '__initcall'
>   - '__exitcall'
> 
> ---
> A vector of macros that should be interpreted as complete statements.
> Typical macros are expressions, and require a semi-colon to be added; sometimes this is not the case, and this allows
> to make clang-format aware of such cases.
> 
> While I was writing this, I’ve found that from ‘DEFINE_XEN_GUEST_HANDLE’ until the end of the list, probably I
> shouldn’t list these entries because all of them end with semi-colon.

Ah, just wanted to ask. I agree that we'd better have here only items
which truly require an exception.

Jan

Re: Clang-format configuration discussion - pt 1

[Luca Fancellu]

> On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 08.11.2023 10:53, Luca Fancellu wrote:
> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>> 
>> Standard: C++03
>> 
>> ---
>> From the documentation: Parse and format C++ constructs compatible with this standard.
> 
> Since I continue to be puzzled - iirc you said this is because of lack
> of availability of "C99" as a value here. What's entirely unclear to
> me is: How does this matter to a tool checking coding style (which is
> largely about formatting, not any lexical or syntactical aspects)?
> 
>> This value is used also in Linux.
> 
> Considering how different the two styles are, I don't think this is
> overly relevant.

Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
of not specifying it at all?

If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
confidence.

However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
the current output. I can’t say that for a different clang-format version though or if changes happen in the
future.


> 
> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>> 
>> AttributeMacros:
>>  - '__init'
>>  - '__exit'
>>  - '__initdata'
>>  - '__initconst'
>>  - '__initconstrel'
>>  - '__initdata_cf_clobber'
>>  - '__initconst_cf_clobber'
>>  - '__hwdom_init'
>>  - '__hwdom_initdata'
>>  - '__maybe_unused'
>>  - '__packed'
>>  - '__stdcall'
>>  - '__vfp_aligned'
>>  - '__alt_call_maybe_initdata'
>>  - '__cacheline_aligned'
>>  - '__ro_after_init'
>>  - 'always_inline'
>>  - 'noinline'
>>  - 'noreturn'
>>  - '__weak'
>>  - '__inline__'
>>  - '__attribute_const__'
>>  - '__transparent__'
>>  - '__used'
>>  - '__must_check'
>>  - '__kprobes'
>> 
>> ---
>> A vector of strings that should be interpreted as attributes/qualifiers instead of identifiers.
>> I’ve tried to list all the attributes I’ve found.
> 
> Like above, the significance of having this list and needing to keep it
> up-to-date is unclear to me. I guess the same also applies to a few
> further settings down from here.

From what I understand, we should give to the formatter tool all the hint possible to make it do a good
job, for example here we should maintain a list of our attributes so that clang-format doesn’t think the function
below is called __init instead of device_tree_node_matches.

static bool __init device_tree_node_matches(const void *fdt, int node, const char *match)
{ ... }

> 
> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>> 
>> StatementMacros:
>>  - 'PROGRESS'
>>  - 'PROGRESS_VCPU'
>>  - 'bitop'
>>  - 'guest_bitop'
>>  - 'testop'
>>  - 'guest_testop'
>>  - 'DEFINE_XEN_GUEST_HANDLE'
>>  - '__DEFINE_XEN_GUEST_HANDLE'
>>  - '___DEFINE_XEN_GUEST_HANDLE'
>>  - 'presmp_initcall'
>>  - '__initcall'
>>  - '__exitcall'
>> 
>> ---
>> A vector of macros that should be interpreted as complete statements.
>> Typical macros are expressions, and require a semi-colon to be added; sometimes this is not the case, and this allows
>> to make clang-format aware of such cases.
>> 
>> While I was writing this, I’ve found that from ‘DEFINE_XEN_GUEST_HANDLE’ until the end of the list, probably I
>> shouldn’t list these entries because all of them end with semi-colon.
> 
> Ah, just wanted to ask. I agree that we'd better have here only items
> which truly require an exception.

Yes my mistake, I’ll fix the list.

> 
> Jan

Re: Clang-format configuration discussion - pt 1

[Alejandro Vallejo]

On Mon, Nov 13, 2023 at 03:20:53PM +0000, Luca Fancellu wrote:
> 
> 
> > On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 08.11.2023 10:53, Luca Fancellu wrote:
> > --------------------------------------------------------------------------------------------------------------------------------------------------------------
> >> 
> >> Standard: C++03
> >> 
> >> ---
> >> From the documentation: Parse and format C++ constructs compatible with this standard.
> > 
> > Since I continue to be puzzled - iirc you said this is because of lack
> > of availability of "C99" as a value here. What's entirely unclear to
> > me is: How does this matter to a tool checking coding style (which is
> > largely about formatting, not any lexical or syntactical aspects)?
> > 
> >> This value is used also in Linux.
> > 
> > Considering how different the two styles are, I don't think this is
> > overly relevant.
On C it _shouldn't_ matter because it's meant to affect C++ constructs
only. That said, clang-format doesn't understand (or care) whether the code
is C or C++, because C's syntax is strictly contained in that of C++ as far
as formatting cares.

While I agree it feels wrong to apply a C++ policy to a C project, it's
largely irrelevant. Setting a value here gives more deterministic output
because it fixes several options' default settings. One would hope none of
those settings affect C, but the world is complex and it's better to be
safe than sorry. Particularly when it's an inocuous one-liner.

There aren't strict C values. And Latest or Auto are simply shorthands for
one of the C++ options.

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

> 
> Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
> of not specifying it at all?
> 
> If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
> for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
> surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
> confidence.
> 
> However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
> the current output. I can’t say that for a different clang-format version though or if changes happen in the
> future.
> 
IMO, C++03 is as good as any other. As long as it's a fixed one.

Cheers,
Alejandro

Re: Clang-format configuration discussion - pt 1

[Jan Beulich]

On 13.11.2023 16:20, Luca Fancellu wrote:
>> On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
>> On 08.11.2023 10:53, Luca Fancellu wrote:
>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>
>>> Standard: C++03
>>>
>>> ---
>>> From the documentation: Parse and format C++ constructs compatible with this standard.
>>
>> Since I continue to be puzzled - iirc you said this is because of lack
>> of availability of "C99" as a value here. What's entirely unclear to
>> me is: How does this matter to a tool checking coding style (which is
>> largely about formatting, not any lexical or syntactical aspects)?
>>
>>> This value is used also in Linux.
>>
>> Considering how different the two styles are, I don't think this is
>> overly relevant.
> 
> Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
> of not specifying it at all?

Not really, no. Here I was merely saying that with the styles being
sufficiently different, what Linux uses is probably not very significant
for our own decision.

> If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
> for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
> surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
> confidence.
> 
> However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
> the current output. I can’t say that for a different clang-format version though or if changes happen in the
> future.

It's fine to set values. All I'm saying is that at least I would prefer
if it was also clear what exact effect the setting of a value has,
especially when that does not really match the language we use in the
project.

>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>
>>> AttributeMacros:
>>>  - '__init'
>>>  - '__exit'
>>>  - '__initdata'
>>>  - '__initconst'
>>>  - '__initconstrel'
>>>  - '__initdata_cf_clobber'
>>>  - '__initconst_cf_clobber'
>>>  - '__hwdom_init'
>>>  - '__hwdom_initdata'
>>>  - '__maybe_unused'
>>>  - '__packed'
>>>  - '__stdcall'
>>>  - '__vfp_aligned'
>>>  - '__alt_call_maybe_initdata'
>>>  - '__cacheline_aligned'
>>>  - '__ro_after_init'
>>>  - 'always_inline'
>>>  - 'noinline'
>>>  - 'noreturn'
>>>  - '__weak'
>>>  - '__inline__'
>>>  - '__attribute_const__'
>>>  - '__transparent__'
>>>  - '__used'
>>>  - '__must_check'
>>>  - '__kprobes'
>>>
>>> ---
>>> A vector of strings that should be interpreted as attributes/qualifiers instead of identifiers.
>>> I’ve tried to list all the attributes I’ve found.
>>
>> Like above, the significance of having this list and needing to keep it
>> up-to-date is unclear to me. I guess the same also applies to a few
>> further settings down from here.
> 
> From what I understand, we should give to the formatter tool all the hint possible to make it do a good
> job, for example here we should maintain a list of our attributes so that clang-format doesn’t think the function
> below is called __init instead of device_tree_node_matches.
> 
> static bool __init device_tree_node_matches(const void *fdt, int node, const char *match)
> { ... }

Well, perhaps they indeed need to do some level of syntax analysis, in
which case knowing which identifiers act as attributes is likely going
to help. Which is where the "needs keeping up-to-date" aspect comes into
play. The example above is simple enough that I wouldn't think a parser
needs to guess what this represents, but presumably there are cases
where ambiguities might arise. (I also wouldn't exclude that the more
involved syntax in C++ increases the desire to have apriori knowledge on
the purpose of certain identifiers. In the end, as per above, the tool
is being told to expect C++ code.)

Jan

Re: Clang-format configuration discussion - pt 1

[Luca Fancellu]

> On 13 Nov 2023, at 16:27, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 13.11.2023 16:20, Luca Fancellu wrote:
>>> On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 08.11.2023 10:53, Luca Fancellu wrote:
>>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>> 
>>>> Standard: C++03
>>>> 
>>>> ---
>>>> From the documentation: Parse and format C++ constructs compatible with this standard.
>>> 
>>> Since I continue to be puzzled - iirc you said this is because of lack
>>> of availability of "C99" as a value here. What's entirely unclear to
>>> me is: How does this matter to a tool checking coding style (which is
>>> largely about formatting, not any lexical or syntactical aspects)?
>>> 
>>>> This value is used also in Linux.
>>> 
>>> Considering how different the two styles are, I don't think this is
>>> overly relevant.
>> 
>> Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
>> of not specifying it at all?
> 
> Not really, no. Here I was merely saying that with the styles being
> sufficiently different, what Linux uses is probably not very significant
> for our own decision.
> 
>> If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
>> for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
>> surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
>> confidence.
>> 
>> However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
>> the current output. I can’t say that for a different clang-format version though or if changes happen in the
>> future.
> 
> It's fine to set values. All I'm saying is that at least I would prefer
> if it was also clear what exact effect the setting of a value has,
> especially when that does not really match the language we use in the
> project.

Yes I agree, I think Alejandro’s reply to this configurable reflects my thoughts about it.

So if we all agree that we should set this parameter, do we all agree that it should be the
value above?

Do you have other concerns regarding this or the other parameters in this thread?

Re: Clang-format configuration discussion - pt 1

[Alejandro Vallejo]

Hi,

On Tue, Nov 14, 2023 at 02:59:35PM +0000, Luca Fancellu wrote:
> 
> 
> > On 13 Nov 2023, at 16:27, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 13.11.2023 16:20, Luca Fancellu wrote:
> >>> On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
> >>> On 08.11.2023 10:53, Luca Fancellu wrote:
> >>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
> >>>> 
> >>>> Standard: C++03
> >>>> 
> >>>> ---
> >>>> From the documentation: Parse and format C++ constructs compatible with this standard.
> >>> 
> >>> Since I continue to be puzzled - iirc you said this is because of lack
> >>> of availability of "C99" as a value here. What's entirely unclear to
> >>> me is: How does this matter to a tool checking coding style (which is
> >>> largely about formatting, not any lexical or syntactical aspects)?
> >>> 
> >>>> This value is used also in Linux.
> >>> 
> >>> Considering how different the two styles are, I don't think this is
> >>> overly relevant.
> >> 
> >> Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
> >> of not specifying it at all?
> > 
> > Not really, no. Here I was merely saying that with the styles being
> > sufficiently different, what Linux uses is probably not very significant
> > for our own decision.
> > 
> >> If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
> >> for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
> >> surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
> >> confidence.
> >> 
> >> However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
> >> the current output. I can’t say that for a different clang-format version though or if changes happen in the
> >> future.
> > 
> > It's fine to set values. All I'm saying is that at least I would prefer
> > if it was also clear what exact effect the setting of a value has,
> > especially when that does not really match the language we use in the
> > project.
On C, allegedly, none. It ought to control defaults for things like
SpaceBeforeCpp11BracedList, SpacesInAngles and other C++-specific things,
because the C++ language sticks syntactical extensions every other Tuesday.
Alas, whatever it does (there's no full list). I'd feel a lot more
comfortable knowing it won't change under our feet.

For reference, clang-format's docs state as an example:

```
c++03:                                 latest:
vector<set<int> > x;           vs.     vector<set<int>> x;
```

> 
> Yes I agree, I think Alejandro’s reply to this configurable reflects my thoughts about it.
> 
> So if we all agree that we should set this parameter, do we all agree that it should be the
> value above?
> 
> Do you have other concerns regarding this or the other parameters in this thread?
> 
> 

Cheers,
Alejandro

Re: Clang-format configuration discussion - pt 1

[Jan Beulich]

On 14.11.2023 15:59, Luca Fancellu wrote:
> 
> 
>> On 13 Nov 2023, at 16:27, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 13.11.2023 16:20, Luca Fancellu wrote:
>>>> On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 08.11.2023 10:53, Luca Fancellu wrote:
>>>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>>>
>>>>> Standard: C++03
>>>>>
>>>>> ---
>>>>> From the documentation: Parse and format C++ constructs compatible with this standard.
>>>>
>>>> Since I continue to be puzzled - iirc you said this is because of lack
>>>> of availability of "C99" as a value here. What's entirely unclear to
>>>> me is: How does this matter to a tool checking coding style (which is
>>>> largely about formatting, not any lexical or syntactical aspects)?
>>>>
>>>>> This value is used also in Linux.
>>>>
>>>> Considering how different the two styles are, I don't think this is
>>>> overly relevant.
>>>
>>> Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
>>> of not specifying it at all?
>>
>> Not really, no. Here I was merely saying that with the styles being
>> sufficiently different, what Linux uses is probably not very significant
>> for our own decision.
>>
>>> If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
>>> for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
>>> surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
>>> confidence.
>>>
>>> However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
>>> the current output. I can’t say that for a different clang-format version though or if changes happen in the
>>> future.
>>
>> It's fine to set values. All I'm saying is that at least I would prefer
>> if it was also clear what exact effect the setting of a value has,
>> especially when that does not really match the language we use in the
>> project.
> 
> Yes I agree, I think Alejandro’s reply to this configurable reflects my thoughts about it.
> 
> So if we all agree that we should set this parameter, do we all agree that it should be the
> value above?
> 
> Do you have other concerns regarding this or the other parameters in this thread?

I did raise what was occurring to me. This doesn't mean that down the
road yet something else might not pop up.

Jan

Re: Clang-format configuration discussion - pt 1

[Luca Fancellu]

> On 14 Nov 2023, at 15:59, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 14.11.2023 15:59, Luca Fancellu wrote:
>> 
>> 
>>> On 13 Nov 2023, at 16:27, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 13.11.2023 16:20, Luca Fancellu wrote:
>>>>> On 13 Nov 2023, at 11:31, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 08.11.2023 10:53, Luca Fancellu wrote:
>>>>> --------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>>>> 
>>>>>> Standard: C++03
>>>>>> 
>>>>>> ---
>>>>>> From the documentation: Parse and format C++ constructs compatible with this standard.
>>>>> 
>>>>> Since I continue to be puzzled - iirc you said this is because of lack
>>>>> of availability of "C99" as a value here. What's entirely unclear to
>>>>> me is: How does this matter to a tool checking coding style (which is
>>>>> largely about formatting, not any lexical or syntactical aspects)?
>>>>> 
>>>>>> This value is used also in Linux.
>>>>> 
>>>>> Considering how different the two styles are, I don't think this is
>>>>> overly relevant.
>>>> 
>>>> Ok, maybe I understand your point, you are looking for a reason to declare this configurable instead
>>>> of not specifying it at all?
>>> 
>>> Not really, no. Here I was merely saying that with the styles being
>>> sufficiently different, what Linux uses is probably not very significant
>>> for our own decision.
>>> 
>>>> If it’s that, from what I understand clang-format will use the default value if we don’t specify anything
>>>> for this one, so it will take ‘Latest’. I think we should put a value for this one to fix it and don’t have
>>>> surprises if that behaviour changes and seeing that also in Linux that value is fixed increased my
>>>> confidence.
>>>> 
>>>> However, if you feel that we should not specify it, I’ve done a test and not specifying it is not changing
>>>> the current output. I can’t say that for a different clang-format version though or if changes happen in the
>>>> future.
>>> 
>>> It's fine to set values. All I'm saying is that at least I would prefer
>>> if it was also clear what exact effect the setting of a value has,
>>> especially when that does not really match the language we use in the
>>> project.
>> 
>> Yes I agree, I think Alejandro’s reply to this configurable reflects my thoughts about it.
>> 
>> So if we all agree that we should set this parameter, do we all agree that it should be the
>> value above?
>> 
>> Do you have other concerns regarding this or the other parameters in this thread?
> 
> I did raise what was occurring to me. This doesn't mean that down the
> road yet something else might not pop up.

Sure, thanks for raising your concern, I’ve asked so that at the deadline if no other concern
are raised, we can move on with another set of configurable to discuss.

Re: Clang-format configuration discussion - pt 1

[George Dunlap]

On Wed, Nov 8, 2023 at 9:53 AM Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>
> Hi all,
>
> Let’s kick off the discussion about clang-format configuration, with this part 1 I would like to discuss some configurable
> that I feel are not controversial.
>
> You can find the serie introducing clang-format here:
> https://patchwork.kernel.org/project/xen-devel/cover/20231031132304.2573924-1-luca.fancellu@arm.com/
> and there is also a patch linked to my gitlab account where you can find the output for the hypervisor code.
>
> For a full list of configurables and to find the possible values for them, please refer to this page:
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Luca,

Thank you so much for the work that you've done here.

The approach in your v2 series looks plausible, as does a brief
overview of the items in this list.

One problem I have is how to really evaluate the proposed changes.  I
spent a few minutes skimming through the "megadiff" [1] output from
the v2 series, and while everything looked fine, that is a HUGE patch
to skim through.  I don't really have any way to know if there's some
rule introduced that I don't really agree with.

On the other hand, I want to avoid busy make-work and the invitation
to interminable bike-shedding discussions.

Is it possible, for instance, to start with a diff which will enforce
*just these settings* (column width, indentation, and so on)?  And
then add on new coding style changes one (or a few) at a time, in a
way that would make it easier to understand what effect each change is
having?  If so, do you think that's a reasonable approach?

If not, how do you propose to proceed?

Thanks,
 -George

[1] https://gitlab.com/luca.fancellu/xen-clang/-/commit/8938bf2196be66b05693a48752ebbdf363e8d8e1.patch

Re: Clang-format configuration discussion - pt 1

[Luca Fancellu]

Hi George,

Thanks a lot for taking the time to have a look on that.

> 
> Luca,
> 
> Thank you so much for the work that you've done here.
> 
> The approach in your v2 series looks plausible, as does a brief
> overview of the items in this list.
> 
> One problem I have is how to really evaluate the proposed changes.  I
> spent a few minutes skimming through the "megadiff" [1] output from
> the v2 series, and while everything looked fine, that is a HUGE patch
> to skim through.  I don't really have any way to know if there's some
> rule introduced that I don't really agree with.
> 
> On the other hand, I want to avoid busy make-work and the invitation
> to interminable bike-shedding discussions.
> 
> Is it possible, for instance, to start with a diff which will enforce
> *just these settings* (column width, indentation, and so on)?  And
> then add on new coding style changes one (or a few) at a time, in a
> way that would make it easier to understand what effect each change is
> having?  If so, do you think that's a reasonable approach?
> 
> If not, how do you propose to proceed?

Yes they are a lot of modifications, the issue is that when we don’t specify
a configurable, the default option will take place, so it’s not really feasible
to produce an output where only the specified configurable will affect the
format.

The easiest, but difficult at the same time, way I thought we can proceed is
discussing a set of rule at the time where we all (well the maintainers) agree
in principle, so that we apply them to the codebase until the list is completed
and also CODING_STYLE can reflect them.

Anyway if someone came up with a better idea, I’m open to suggestions.

Cheers,
Luca