> On 24 Nov 2023, at 11:12, George Dunlap <[email protected]> wrote: > > On Thu, Nov 23, 2023 at 2:48 PM Luca Fancellu <[email protected]> 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/[email protected]/#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
