Re: Clang-format configuration discussion - pt 2

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 24 Nov 2023, at 11:12, George Dunlap <george.dunlap@xxxxxxxxx> wrote:
> 
> On Thu, Nov 23, 2023 at 2:48 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>> AlignConsecutiveAssignments: None
>> 
>> ---
>> This one is disabled because of feedbacks from Stefano and Alejandro about 
>> some weird behaviour on our
>> codebase.
>> 
>> This one could be phased along this line: “Consecutive assignments don't 
>> need to be aligned.”, the issue is
>> that in this way it seems that it’s optional, but clang-format is going to 
>> remove the alignment anyway for
>> assignment that are consecutive and aligned.
> 
> It's hard to agree on this one without seeing some of the examples of
> what it does, some examples of the "weird behavior" Stefano &
> Allejandro found,

I think there was a comment from Stefano for the RFC v1:
https://patchwork.kernel.org/project/xen-devel/cover/20230728081144.4124309-1-luca.fancellu@xxxxxxx/#25447625

> and some examples of places where it's going to
> remove the alignment.

Hi George,

You are right, with an example is better, so I’ve checked into the output:
https://gitlab.com/luca.fancellu/xen-clang/-/commit/8938bf2196be66b05693a48752ebbdf363e8d8e1.patch

And for this one, you can see here on line 6186 of that patch:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 49792dd590..c229318450 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c

[...]

@@ -3333,19 +3318,18 @@ static int __init alloc_domain_evtchn(struct 
dt_device_node *node)
rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
if ( rc < 0 )
{
- printk(XENLOG_ERR
- "evtchn_alloc_unbound() failure (Error %d) \n", rc);
+ printk(XENLOG_ERR "evtchn_alloc_unbound() failure (Error %d) \n", rc);
return rc;
}
- bind_interdomain.remote_dom = d1->domain_id;
+ bind_interdomain.remote_dom = d1->domain_id;
bind_interdomain.remote_port = domU1_port;
rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
if ( rc < 0 )
{

Assignment of bind_interdomain.remote_dom was aligned with the following line, 
but since the value
of this configurable is “None”, clang-format is modifying that to use only one 
space before the assignment
operator.



One example related to macros can be found on line 1663:

diff --git a/xen/arch/arm/arm32/insn.c b/xen/arch/arm/arm32/insn.c
index 49953a042a..1635c4b6d1 100644
--- a/xen/arch/arm/arm32/insn.c
+++ b/xen/arch/arm/arm32/insn.c
@@ -19,9 +19,9 @@
#include <asm/insn.h>
/* Mask of branch instructions' immediate. */
-#define BRANCH_INSN_IMM_MASK GENMASK(23, 0)
+#define BRANCH_INSN_IMM_MASK GENMASK(23, 0)
/* Shift of branch instructions' immediate. */
-#define BRANCH_INSN_IMM_SHIFT 0
+#define BRANCH_INSN_IMM_SHIFT 0

Clang format sees the comment between BRANCH_INSN_IMM_MASK and 
BRANCH_INSN_IMM_SHIFT and
even if before the value of the macros were aligned, it applies the rule of one 
space between the macro name
and the value.

> 
> I had tried to apply your series before and didn't get very far with
> it for some reason ISTR.

If you reach me we can see what issue you are facing, I think staging went 
ahead since my last push,
but I’ve put a SHA in the cover letter, anyway I can help on that if you want.


>  One way to see the effect of individual
> features would be:
> 
> 1. Make a branch with one big patch applying clang-format for a given style
> 
> 2. Change just one style line, re-run clang format, and create a new
> patch from that
> 
> Then it would be easy to see the difference between the two.  It might
> actually be easier for individual reviewers to do that on their own
> trees, rather than to ask you to try to generate and post such patches
> somewhere.
> 
> -George