Coding Style Review and Automation

Coding Style Review and Automation

[Oleksandr Andrushchenko]

Hello, everybody!

What is the rationale behind introducing a tool to help with coding style
verification? I will partially quote Viktor Mitin here [2]:

"The Xen Project has a coding standard in place, but like many projects, the
standard is only enforced through peer review. Such mistakes slip through and
code is imported from other projects which may not follow the same standard. The
goal would be to come up with a tool that can audit the code base as part of a
CI loop for code style inconsistencies and potentially provide corrections. This
tool is to embed as a part of the continuous integration loop."

I would add that it would better reflect reality to say that Xen's coding style
is quite incomplete to become a standard and needs some improvement to achieve
that.

Here, I would like to provide a bit of history and acknowledge those brave
individuals who dared and tried to introduce to Xen coding style checking and
formatting support with clang-format.

Year 2017, Ishani Chugh.
---------------------------------------------------------------------
This journey began with a request from an Outreachy program member [1].
Then came the first patches by Ishani Chugh [2]
Status: *busted*.

Year 2019, Viktor Mitin
---------------------------------------------------------------------
Then picked up by Viktor Mitin, EPAM's first attempt [3].
Status: *busted*.

Year 2020, Anastasiia Lukianenko
---------------------------------------------------------------------
Continued by Anastasiia Lukianenko, EPAM's second attempt [4], [5].
Some contributions were made to LLVM to make clang-format a better fit for
Xen [6].
Xen-summit and presentation [7] and the summary document [8].
Status: *busted*.

Year 2023, Luca Fancellu
---------------------------------------------------------------------
Luca restarted it, first ARM attempt [9], [10], [11].
Status: *busted*.

That's all for now, but it is still impressive as of 2025.

So, in my opinion, what were the main issues with all these attempts? There are
many different views, but I would emphasize the following:

1) clang-format doesn't perfectly fit Xen's code style as some rules it applies
are not liked by the community or it applies rules that Xen's coding style
doesn't define (those Luca described in his .clang-format for every clang
option).

2) clang-format doesn't work in a "one-option-at-a-time" mode [12]: clang
maintainers strongly oppose requests to allow turning off all options except
some. Believe it or not, other maintainers also have strong opinions on what is
right and what is not for their projects, and this is one area where they will
not compromise.

3) The size of the patch after applying clang-format is huge. Really. Something
like 9 MB. Even if everyone agrees that the approach is good and we can proceed
with it, it is highly unlikely anyone will be able to review it. Considering
that new patches are being added to the upstream during such a review, it may
also lead to new code style violations or require a new review of that huge
patch.

4) Which clang-format version should we set as the one used by Xen, so it is
easy for everyone to use it on their hosts?

5) You name it. I think many people in the community can name their points for
and against clang-format.

So, in this attempt, I would suggest the following approach:
I think that I could start sending patches after the latest .clang-format 21 for
some part of Xen, ARM code base for example, using work already done by Luca.
This way:

1) Patches are formatted with clang-format, which is a strong plus.
2) The diff is well maintained and I can still alter it by hand if there are
comments or dislikes.
3) Finally, when the patch is accepted, we can be more confident that
clang-format will find far fewer inconsistencies than if it were just applied to
the "raw" code. Thus, the next time clang-format runs, it will produce a much
smaller patch than before.
4) Finally, introduce clang-format and run it on the leftovers: at this stage,
it would be much easier to discuss every option clang has and the resulting
output it produces.
5) Update existing/add new rules to the Xen coding style based on community
agreements and the results of this activity.

We may define the subsystems to start this activity on and also define an
acceptable size of the patch for review, say 100K. Considering that the longer
the review, the more outdated the patch becomes and will require a new round as
new code comes in.

I would love to hear from the community on this approach and finally get it
done. Not busted.

Stay safe,
Oleksandr Andrushchenko

[1] https://lore.kernel.org/xen-devel/1130763396.5603480.1492372859631.JavaMail.zimbra@research.iiit.ac.in/T/#m1db2521362edd286875acf10296873993226dcf2
[2] https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01739.html
[3] https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01862.html
[4] https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg02157.html
[5] https://lists.xenproject.org/archives/html/xen-devel/2020-10/msg00022.html
[6] https://reviews.llvm.org/D91950
[7] https://xenproject.org/blog/clang-format-for-xen-coding-style-checking-scheduled/
[8] https://docs.google.com/document/d/1MDzYkPgfVpI_UuO_3NRXsRLAXqIZ6pj2btF7vsMYj8o
[9] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02294.html
[10] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg00498.html
[11] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01993.html
[12] https://github.com/llvm/llvm-project/issues/54137#issuecomment-1058564570

Re: Coding Style Review and Automation

[Stefano Stabellini]

Thanks Oleksandr for the summary. Adding a few maintainers in CC

On Mon, 10 Feb 2025, Oleksandr Andrushchenko wrote:
> Hello, everybody!
> 
> What is the rationale behind introducing a tool to help with coding style
> verification? I will partially quote Viktor Mitin here [2]:
> 
> "The Xen Project has a coding standard in place, but like many projects, the
> standard is only enforced through peer review. Such mistakes slip through and
> code is imported from other projects which may not follow the same standard.
> The
> goal would be to come up with a tool that can audit the code base as part of a
> CI loop for code style inconsistencies and potentially provide corrections.
> This
> tool is to embed as a part of the continuous integration loop."
> 
> I would add that it would better reflect reality to say that Xen's coding
> style
> is quite incomplete to become a standard and needs some improvement to achieve
> that.
> 
> Here, I would like to provide a bit of history and acknowledge those brave
> individuals who dared and tried to introduce to Xen coding style checking and
> formatting support with clang-format.
> 
> Year 2017, Ishani Chugh.
> ---------------------------------------------------------------------
> This journey began with a request from an Outreachy program member [1].
> Then came the first patches by Ishani Chugh [2]
> Status: *busted*.
> 
> Year 2019, Viktor Mitin
> ---------------------------------------------------------------------
> Then picked up by Viktor Mitin, EPAM's first attempt [3].
> Status: *busted*.
> 
> Year 2020, Anastasiia Lukianenko
> ---------------------------------------------------------------------
> Continued by Anastasiia Lukianenko, EPAM's second attempt [4], [5].
> Some contributions were made to LLVM to make clang-format a better fit for
> Xen [6].
> Xen-summit and presentation [7] and the summary document [8].
> Status: *busted*.
> 
> Year 2023, Luca Fancellu
> ---------------------------------------------------------------------
> Luca restarted it, first ARM attempt [9], [10], [11].
> Status: *busted*.
> 
> That's all for now, but it is still impressive as of 2025.
> 
> So, in my opinion, what were the main issues with all these attempts? There
> are
> many different views, but I would emphasize the following:
> 
> 1) clang-format doesn't perfectly fit Xen's code style as some rules it
> applies
> are not liked by the community or it applies rules that Xen's coding style
> doesn't define (those Luca described in his .clang-format for every clang
> option).
> 
> 2) clang-format doesn't work in a "one-option-at-a-time" mode [12]: clang
> maintainers strongly oppose requests to allow turning off all options except
> some. Believe it or not, other maintainers also have strong opinions on what
> is
> right and what is not for their projects, and this is one area where they will
> not compromise.
> 
> 3) The size of the patch after applying clang-format is huge. Really.
> Something
> like 9 MB. Even if everyone agrees that the approach is good and we can
> proceed
> with it, it is highly unlikely anyone will be able to review it. Considering
> that new patches are being added to the upstream during such a review, it may
> also lead to new code style violations or require a new review of that huge
> patch.
> 
> 4) Which clang-format version should we set as the one used by Xen, so it is
> easy for everyone to use it on their hosts?
> 
> 5) You name it. I think many people in the community can name their points for
> and against clang-format.
> 
> So, in this attempt, I would suggest the following approach:
> I think that I could start sending patches after the latest .clang-format 21
> for
> some part of Xen, ARM code base for example, using work already done by Luca.
> This way:
> 
> 1) Patches are formatted with clang-format, which is a strong plus.
> 2) The diff is well maintained and I can still alter it by hand if there are
> comments or dislikes.
> 3) Finally, when the patch is accepted, we can be more confident that
> clang-format will find far fewer inconsistencies than if it were just applied
> to
> the "raw" code. Thus, the next time clang-format runs, it will produce a much
> smaller patch than before.
> 4) Finally, introduce clang-format and run it on the leftovers: at this stage,
> it would be much easier to discuss every option clang has and the resulting
> output it produces.
> 5) Update existing/add new rules to the Xen coding style based on community
> agreements and the results of this activity.
> 
> We may define the subsystems to start this activity on and also define an
> acceptable size of the patch for review, say 100K. Considering that the longer
> the review, the more outdated the patch becomes and will require a new round
> as
> new code comes in.
> 
> I would love to hear from the community on this approach and finally get it
> done. Not busted.
> 
> Stay safe,
> Oleksandr Andrushchenko
> 
> [1]
> https://lore.kernel.org/xen-devel/1130763396.5603480.1492372859631.JavaMail.zimbra@research.iiit.ac.in/T/#m1db2521362edd286875acf10296873993226dcf2
> [2] https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01739.html
> [3] https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01862.html
> [4] https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg02157.html
> [5] https://lists.xenproject.org/archives/html/xen-devel/2020-10/msg00022.html
> [6] https://reviews.llvm.org/D91950
> [7]
> https://xenproject.org/blog/clang-format-for-xen-coding-style-checking-scheduled/
> [8]
> https://docs.google.com/document/d/1MDzYkPgfVpI_UuO_3NRXsRLAXqIZ6pj2btF7vsMYj8o
> [9] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02294.html
> [10]
> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg00498.html
> [11]
> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01993.html
> [12] https://github.com/llvm/llvm-project/issues/54137#issuecomment-1058564570
> 

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Mon, Feb 10, 2025 at 11:16:09PM +0200, Oleksandr Andrushchenko wrote:
> Hello, everybody!
> 
> What is the rationale behind introducing a tool to help with coding style
> verification? I will partially quote Viktor Mitin here [2]:
> 
> "The Xen Project has a coding standard in place, but like many projects, the
> standard is only enforced through peer review. Such mistakes slip through and
> code is imported from other projects which may not follow the same standard. The
> goal would be to come up with a tool that can audit the code base as part of a
> CI loop for code style inconsistencies and potentially provide corrections. This
> tool is to embed as a part of the continuous integration loop."
> 
> I would add that it would better reflect reality to say that Xen's coding style
> is quite incomplete to become a standard and needs some improvement to achieve
> that.
> 
> Here, I would like to provide a bit of history and acknowledge those brave
> individuals who dared and tried to introduce to Xen coding style checking and
> formatting support with clang-format.
> 
> Year 2017, Ishani Chugh.
> ---------------------------------------------------------------------
> This journey began with a request from an Outreachy program member [1].
> Then came the first patches by Ishani Chugh [2]
> Status: *busted*.
> 
> Year 2019, Viktor Mitin
> ---------------------------------------------------------------------
> Then picked up by Viktor Mitin, EPAM's first attempt [3].
> Status: *busted*.
> 
> Year 2020, Anastasiia Lukianenko
> ---------------------------------------------------------------------
> Continued by Anastasiia Lukianenko, EPAM's second attempt [4], [5].
> Some contributions were made to LLVM to make clang-format a better fit for
> Xen [6].
> Xen-summit and presentation [7] and the summary document [8].
> Status: *busted*.
> 
> Year 2023, Luca Fancellu
> ---------------------------------------------------------------------
> Luca restarted it, first ARM attempt [9], [10], [11].
> Status: *busted*.
> 
> That's all for now, but it is still impressive as of 2025.
> 
> So, in my opinion, what were the main issues with all these attempts? There are
> many different views, but I would emphasize the following:
> 
> 1) clang-format doesn't perfectly fit Xen's code style as some rules it applies
> are not liked by the community or it applies rules that Xen's coding style
> doesn't define (those Luca described in his .clang-format for every clang
> option).
> 
> 2) clang-format doesn't work in a "one-option-at-a-time" mode [12]: clang
> maintainers strongly oppose requests to allow turning off all options except
> some. Believe it or not, other maintainers also have strong opinions on what is
> right and what is not for their projects, and this is one area where they will
> not compromise.
> 
> 3) The size of the patch after applying clang-format is huge. Really. Something
> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
> with it, it is highly unlikely anyone will be able to review it. Considering
> that new patches are being added to the upstream during such a review, it may
> also lead to new code style violations or require a new review of that huge
> patch.

I think this approach is difficult.  It would likely introduce a lot
of noise when using `git blame` (I know, it's just one extra jump,
but...), plus would likely break every patch that we currently have
in-flight.

> 4) Which clang-format version should we set as the one used by Xen, so it is
> easy for everyone to use it on their hosts?
> 
> 5) You name it. I think many people in the community can name their points for
> and against clang-format.

What are the parts of our coding style that clang-format cannot
correctly represent?  Could you make a list of what would need to
change in Xen coding style for it to match perfectly what clang-format
will check?

Ideally the first step would be to prepare a patch to adjust the
coding style so it's in line with what clang-format will do.

Is it possible for clang-format to be applied exclusively to newly
added chunks of code, while keeping the surroundings untouched?

Thanks, Roger.

Re: Coding Style Review and Automation

[Luca Fancellu]

Hi Roger,

>> 
>> 3) The size of the patch after applying clang-format is huge. Really. Something
>> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
>> with it, it is highly unlikely anyone will be able to review it. Considering
>> that new patches are being added to the upstream during such a review, it may
>> also lead to new code style violations or require a new review of that huge
>> patch.
> 
> I think this approach is difficult.  It would likely introduce a lot
> of noise when using `git blame` (I know, it's just one extra jump,
> but...), plus would likely break every patch that we currently have
> in-flight.

I think we already discussed this in the past and having some churn was accepted,
also about breaking existing patches, every change merged has the potential to do
that, this one is more likely but it’s the game I guess?

> 
>> 4) Which clang-format version should we set as the one used by Xen, so it is
>> easy for everyone to use it on their hosts?
>> 
>> 5) You name it. I think many people in the community can name their points for
>> and against clang-format.
> 
> What are the parts of our coding style that clang-format cannot
> correctly represent?  Could you make a list of what would need to
> change in Xen coding style for it to match perfectly what clang-format
> will check?

we already went through that route, there is no checker anywhere that matches
the Xen coding style perfectly, so it’s either we change the coding style or we
don’t proceed further with any automatic check

> 
> Ideally the first step would be to prepare a patch to adjust the
> coding style so it's in line with what clang-format will do.

It’s easy to say that, but difficult to implement, if we could accept the clang-format
rules it would be easier to adopt the configuration itself as coding style, maybe
enhanced with some comments.

Cheers,
Luca

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Tue, Feb 11, 2025 at 09:10:38AM +0000, Luca Fancellu wrote:
> Hi Roger,
> 
> >> 
> >> 3) The size of the patch after applying clang-format is huge. Really. Something
> >> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
> >> with it, it is highly unlikely anyone will be able to review it. Considering
> >> that new patches are being added to the upstream during such a review, it may
> >> also lead to new code style violations or require a new review of that huge
> >> patch.
> > 
> > I think this approach is difficult.  It would likely introduce a lot
> > of noise when using `git blame` (I know, it's just one extra jump,
> > but...), plus would likely break every patch that we currently have
> > in-flight.
> 
> I think we already discussed this in the past and having some churn was accepted,
> also about breaking existing patches, every change merged has the potential to do
> that, this one is more likely but it’s the game I guess?

Hm, maybe, I don't get rebasing issues very often TBH.  Not sure how
intrusive such patch would be.

> > 
> >> 4) Which clang-format version should we set as the one used by Xen, so it is
> >> easy for everyone to use it on their hosts?
> >> 
> >> 5) You name it. I think many people in the community can name their points for
> >> and against clang-format.
> > 
> > What are the parts of our coding style that clang-format cannot
> > correctly represent?  Could you make a list of what would need to
> > change in Xen coding style for it to match perfectly what clang-format
> > will check?
> 
> we already went through that route, there is no checker anywhere that matches
> the Xen coding style perfectly, so it’s either we change the coding style or we
> don’t proceed further with any automatic check

I'm probably fine with changing coding style, that's why I'm asking
for a list of what needs to be changed (unless we switch to a
completely different coding style).

> > 
> > Ideally the first step would be to prepare a patch to adjust the
> > coding style so it's in line with what clang-format will do.
> 
> It’s easy to say that, but difficult to implement, if we could accept the clang-format
> rules it would be easier to adopt the configuration itself as coding style, maybe
> enhanced with some comments.

I'm kind of lost, why is it difficult to implement?  What I'm asking
for is a patch to CODING_STYLE that modifies it in a way that we could
use clang-format.  In any case we need to do that if we want to use
clang-format.

One question that seems to have been dropped from my previous email:
would it be feasible to apply the updated style to newly added chunks
of code only, but not to the (unmodified) surrounding context?

Thanks, Roger.

Re: Coding Style Review and Automation

[Luca Fancellu]

Hi Roger,

>>>> 
>>>> 5) You name it. I think many people in the community can name their points for
>>>> and against clang-format.
>>> 
>>> What are the parts of our coding style that clang-format cannot
>>> correctly represent?  Could you make a list of what would need to
>>> change in Xen coding style for it to match perfectly what clang-format
>>> will check?
>> 
>> we already went through that route, there is no checker anywhere that matches
>> the Xen coding style perfectly, so it’s either we change the coding style or we
>> don’t proceed further with any automatic check
> 
> I'm probably fine with changing coding style, that's why I'm asking
> for a list of what needs to be changed (unless we switch to a
> completely different coding style).

Sure, I think listing the differences is fine.

> 
>>> 
>>> Ideally the first step would be to prepare a patch to adjust the
>>> coding style so it's in line with what clang-format will do.
>> 
>> It’s easy to say that, but difficult to implement, if we could accept the clang-format
>> rules it would be easier to adopt the configuration itself as coding style, maybe
>> enhanced with some comments.
> 
> I'm kind of lost, why is it difficult to implement?  What I'm asking
> for is a patch to CODING_STYLE that modifies it in a way that we could
> use clang-format.  In any case we need to do that if we want to use
> clang-format.

Changes to the CODING_STYLE are historically difficult, given that the natural
language is prone to interpretation. I’m not opposing to that, I’m just pointing out that
starting changing the CODING_STYLE in order to accept the clang-format feels
more risky and time consuming than testing clang-format and modifying the
CODING_STYLE accordingly.

Anyway the resulting clang-format would have our coding style for what can be covered
by the tool and have something new for what it cannot be covered, it’s just that at least
you have a reproducible way that can be tested.

> 
> One question that seems to have been dropped from my previous email:
> would it be feasible to apply the updated style to newly added chunks
> of code only, but not to the (unmodified) surrounding context?

I dropped that one from my reply because I know there are tools that do that,
but I’ve never investigated, maybe Oleksandr could try to have a look.

I’m not sure if the result would feel more like a frankenstein, but it would for sure
limit the churn.

Cheers,
Luca

Re: Coding Style Review and Automation

[Jan Beulich]

On 11.02.2025 10:01, Roger Pau Monné wrote:
> Is it possible for clang-format to be applied exclusively to newly
> added chunks of code, while keeping the surroundings untouched?

I, too, was wondering about this, at least as a data point. However,
especially for files that aren't in a single style (e.g. where we're
slowly transitioning from Linux to Xen style) this would then mean
(at least) three styles in a single file.

Jan

Re: Coding Style Review and Automation

[Jan Beulich]

On 11.02.2025 10:10, Luca Fancellu wrote:
>>> 3) The size of the patch after applying clang-format is huge. Really. Something
>>> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
>>> with it, it is highly unlikely anyone will be able to review it. Considering
>>> that new patches are being added to the upstream during such a review, it may
>>> also lead to new code style violations or require a new review of that huge
>>> patch.
>>
>> I think this approach is difficult.  It would likely introduce a lot
>> of noise when using `git blame` (I know, it's just one extra jump,
>> but...), plus would likely break every patch that we currently have
>> in-flight.
> 
> I think we already discussed this in the past and having some churn was accepted,
> also about breaking existing patches, every change merged has the potential to do
> that, this one is more likely but it’s the game I guess?

That's easy to say if you have just a few patches in flight, yet I'm worried
about this when considering the hundreds of mine that are awaiting review.
Of course this would be of almost no concern (to me) if for the experimental
phase it was only Arm code that was converted. Yet even then the plan is going
to be to later do a full conversion (to whatever the worked out final style
is), and I don't foresee the review situation to get any better. It'll be only
yet more patches then that will need re-basing.

Jan

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Tue, Feb 11, 2025 at 09:49:54AM +0000, Luca Fancellu wrote:
> Hi Roger,
> 
> >>>> 
> >>>> 5) You name it. I think many people in the community can name their points for
> >>>> and against clang-format.
> >>> 
> >>> What are the parts of our coding style that clang-format cannot
> >>> correctly represent?  Could you make a list of what would need to
> >>> change in Xen coding style for it to match perfectly what clang-format
> >>> will check?
> >> 
> >> we already went through that route, there is no checker anywhere that matches
> >> the Xen coding style perfectly, so it’s either we change the coding style or we
> >> don’t proceed further with any automatic check
> > 
> > I'm probably fine with changing coding style, that's why I'm asking
> > for a list of what needs to be changed (unless we switch to a
> > completely different coding style).
> 
> Sure, I think listing the differences is fine.
> 
> > 
> >>> 
> >>> Ideally the first step would be to prepare a patch to adjust the
> >>> coding style so it's in line with what clang-format will do.
> >> 
> >> It’s easy to say that, but difficult to implement, if we could accept the clang-format
> >> rules it would be easier to adopt the configuration itself as coding style, maybe
> >> enhanced with some comments.
> > 
> > I'm kind of lost, why is it difficult to implement?  What I'm asking
> > for is a patch to CODING_STYLE that modifies it in a way that we could
> > use clang-format.  In any case we need to do that if we want to use
> > clang-format.
> 
> Changes to the CODING_STYLE are historically difficult, given that the natural
> language is prone to interpretation. I’m not opposing to that, I’m just pointing out that
> starting changing the CODING_STYLE in order to accept the clang-format feels
> more risky and time consuming than testing clang-format and modifying the
> CODING_STYLE accordingly.

I suggested that because it's IMO important to see the resulting
style.  I'm not suggesting to modify CODING_STYLE in a single change,
it should be multiple patches that adjust the different areas that
require changes to match what clang-format can do.

I think it's important that we can see how the final style will look
like, otherwise it's hard to compromise on intermediate seemingly
unconnected changes without knowing what the end result will be.

> Anyway the resulting clang-format would have our coding style for what can be covered
> by the tool and have something new for what it cannot be covered, it’s just that at least
> you have a reproducible way that can be tested.
> 
> > 
> > One question that seems to have been dropped from my previous email:
> > would it be feasible to apply the updated style to newly added chunks
> > of code only, but not to the (unmodified) surrounding context?
> 
> I dropped that one from my reply because I know there are tools that do that,
> but I’ve never investigated, maybe Oleksandr could try to have a look.
> 
> I’m not sure if the result would feel more like a frankenstein, but it would for sure
> limit the churn.

Depends on how many differences there are between our current coding
style and what clang-format can enforce that's closer to our existing
style.

Knowing the differences would be key in understanding whether this
would be a viable approach.  It would certainly be my preference if
the differences between our coding style and what clang-format can do
are not too big.

Thanks, Roger.

Re: Coding Style Review and Automation

[Jan Beulich]

On 10.02.2025 22:16, Oleksandr Andrushchenko wrote:
> So, in this attempt, I would suggest the following approach:
> I think that I could start sending patches after the latest .clang-format 21 for
> some part of Xen, ARM code base for example, using work already done by Luca.

Taking the suggestion of limiting the experiment to Arm: On its upside
it would mean non-Arm folks are unaffected. Yet that's the downside at
the same time: Non-Arm folks then also are unlikely to even look at the
patches. May I suggest to consider e.g. drivers/ instead? That'll involve
more people (to build an opinion) while at the same time it's fewer files
and less code overall.

> This way:
> 
> 1) Patches are formatted with clang-format, which is a strong plus.
> 2) The diff is well maintained and I can still alter it by hand if there are
> comments or dislikes.
> 3) Finally, when the patch is accepted, we can be more confident that
> clang-format will find far fewer inconsistencies than if it were just applied to
> the "raw" code. Thus, the next time clang-format runs, it will produce a much
> smaller patch than before.
> 4) Finally, introduce clang-format and run it on the leftovers: at this stage,
> it would be much easier to discuss every option clang has and the resulting
> output it produces.
> 5) Update existing/add new rules to the Xen coding style based on community
> agreements and the results of this activity.
> 
> We may define the subsystems to start this activity on and also define an
> acceptable size of the patch for review, say 100K. Considering that the longer
> the review, the more outdated the patch becomes and will require a new round as
> new code comes in.

Why even go as big as 100k? Style changes can literally be done at any
granularity. In particular for small enough files they can be done one
file at a time. For larger files there can be multiple steps (preferably
all committed together, but not doing so may still be acceptable).

From the gigantic monolithic patch, would it be possible to fish out some
very representative (for the changes that need making) files and start
from there?

Jan

Re: Coding Style Review and Automation

[Jan Beulich]

On 11.02.2025 11:26, Roger Pau Monné wrote:
> On Tue, Feb 11, 2025 at 09:49:54AM +0000, Luca Fancellu wrote:
>> Hi Roger,
>>
>>>>>>
>>>>>> 5) You name it. I think many people in the community can name their points for
>>>>>> and against clang-format.
>>>>>
>>>>> What are the parts of our coding style that clang-format cannot
>>>>> correctly represent?  Could you make a list of what would need to
>>>>> change in Xen coding style for it to match perfectly what clang-format
>>>>> will check?
>>>>
>>>> we already went through that route, there is no checker anywhere that matches
>>>> the Xen coding style perfectly, so it’s either we change the coding style or we
>>>> don’t proceed further with any automatic check
>>>
>>> I'm probably fine with changing coding style, that's why I'm asking
>>> for a list of what needs to be changed (unless we switch to a
>>> completely different coding style).
>>
>> Sure, I think listing the differences is fine.
>>
>>>
>>>>>
>>>>> Ideally the first step would be to prepare a patch to adjust the
>>>>> coding style so it's in line with what clang-format will do.
>>>>
>>>> It’s easy to say that, but difficult to implement, if we could accept the clang-format
>>>> rules it would be easier to adopt the configuration itself as coding style, maybe
>>>> enhanced with some comments.
>>>
>>> I'm kind of lost, why is it difficult to implement?  What I'm asking
>>> for is a patch to CODING_STYLE that modifies it in a way that we could
>>> use clang-format.  In any case we need to do that if we want to use
>>> clang-format.
>>
>> Changes to the CODING_STYLE are historically difficult, given that the natural
>> language is prone to interpretation. I’m not opposing to that, I’m just pointing out that
>> starting changing the CODING_STYLE in order to accept the clang-format feels
>> more risky and time consuming than testing clang-format and modifying the
>> CODING_STYLE accordingly.
> 
> I suggested that because it's IMO important to see the resulting
> style.  I'm not suggesting to modify CODING_STYLE in a single change,
> it should be multiple patches that adjust the different areas that
> require changes to match what clang-format can do.
> 
> I think it's important that we can see how the final style will look
> like, otherwise it's hard to compromise on intermediate seemingly
> unconnected changes without knowing what the end result will be.

+1 on both points.

Jan

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Tue, Feb 11, 2025 at 11:14:55AM +0100, Jan Beulich wrote:
> On 11.02.2025 10:01, Roger Pau Monné wrote:
> > Is it possible for clang-format to be applied exclusively to newly
> > added chunks of code, while keeping the surroundings untouched?
> 
> I, too, was wondering about this, at least as a data point. However,
> especially for files that aren't in a single style (e.g. where we're
> slowly transitioning from Linux to Xen style) this would then mean
> (at least) three styles in a single file.

My expectation is that the proposed clang-format based style will be
very close to the current Xen style.  Maybe that's unrealistic, that's
why I requested a list of differences between our current coding style
and what clang-format can do.

Thanks, Roger.

Re: Coding Style Review and Automation

[Anthony PERARD]

On Tue, Feb 11, 2025 at 10:31:41AM +0100, Roger Pau Monné wrote:
> One question that seems to have been dropped from my previous email:
> would it be feasible to apply the updated style to newly added chunks
> of code only, but not to the (unmodified) surrounding context?

There's a tool that can format only one patch, `git-clang-format`.

    https://clang.llvm.org/docs/ClangFormat.html#git-integration

This could help temporally, or as a way to format a patch under review
to get a better view of the new coding style (instead of having a whole
file been formatted which could make reviewing the new style harder).

Cheers,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Tue, Feb 11, 2025 at 11:19:23AM +0100, Jan Beulich wrote:
> On 11.02.2025 10:10, Luca Fancellu wrote:
> >>> 3) The size of the patch after applying clang-format is huge. Really. Something
> >>> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
> >>> with it, it is highly unlikely anyone will be able to review it. Considering
> >>> that new patches are being added to the upstream during such a review, it may
> >>> also lead to new code style violations or require a new review of that huge
> >>> patch.
> >>
> >> I think this approach is difficult.  It would likely introduce a lot
> >> of noise when using `git blame` (I know, it's just one extra jump,
> >> but...), plus would likely break every patch that we currently have
> >> in-flight.
> > 
> > I think we already discussed this in the past and having some churn was accepted,
> > also about breaking existing patches, every change merged has the potential to do
> > that, this one is more likely but it’s the game I guess?
> 
> That's easy to say if you have just a few patches in flight, yet I'm worried
> about this when considering the hundreds of mine that are awaiting review.

There are also downstreams (including distros) with varying length of
patch queues on top of Xen.  Arguably they have to rebase the queue
every time they update, but a wide change in coding style will likely
be fairly disruptive to them.

Don't take this as a reason to reject clang-format.  As mentioned
elsewhere I think the format supported by clang-format would need to
be fairly similar to the current Xen one (up to the point that chunks
of code using the new and the old style could live together).  Then we
would enforce it only for newly added chunks of code initially IMO.

Thanks, Roger.

Re: Coding Style Review and Automation

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Tue, Feb 11, 2025 at 03:06:08PM +0100, Roger Pau Monné wrote:
> On Tue, Feb 11, 2025 at 11:19:23AM +0100, Jan Beulich wrote:
> > On 11.02.2025 10:10, Luca Fancellu wrote:
> > >>> 3) The size of the patch after applying clang-format is huge. Really. Something
> > >>> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
> > >>> with it, it is highly unlikely anyone will be able to review it. Considering
> > >>> that new patches are being added to the upstream during such a review, it may
> > >>> also lead to new code style violations or require a new review of that huge
> > >>> patch.
> > >>
> > >> I think this approach is difficult.  It would likely introduce a lot
> > >> of noise when using `git blame` (I know, it's just one extra jump,
> > >> but...), plus would likely break every patch that we currently have
> > >> in-flight.
> > > 
> > > I think we already discussed this in the past and having some churn was accepted,
> > > also about breaking existing patches, every change merged has the potential to do
> > > that, this one is more likely but it’s the game I guess?
> > 
> > That's easy to say if you have just a few patches in flight, yet I'm worried
> > about this when considering the hundreds of mine that are awaiting review.
> 
> There are also downstreams (including distros) with varying length of
> patch queues on top of Xen.  Arguably they have to rebase the queue
> every time they update, but a wide change in coding style will likely
> be fairly disruptive to them.
> 
> Don't take this as a reason to reject clang-format.  As mentioned
> elsewhere I think the format supported by clang-format would need to
> be fairly similar to the current Xen one (up to the point that chunks
> of code using the new and the old style could live together).  Then we
> would enforce it only for newly added chunks of code initially IMO.

While clang-format surely will force some changes (the earlier 9MB patch
is a data point here...), I generally expect it should match fairly
close to the current code style, and so the rebase shouldn't be _that_
painful. In some cases git likely will handle large part of the work
already.

It surely is a cost of introducing such a change, but IMO it's a cost
worth paying.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Coding Style Review and Automation

[Stefano Stabellini]

Hi Oleksandr,

This morning, we had a discussion among maintainers, and the suggested
approach moving forward is as follows:

- First, it would be helpful to see a sample of the proposed changes
  applied to a single source file as an example. If you could provide
  such a patch, it would help advance the discussion.

- If the changes are acceptable, we need to properly document the new
  coding style in xen.git. If not, we will need to iterate again. We may
  also need to add a "xen" template to clang-format.

- Once finalized, we will proceed by making changes to the Xen source
  code piece by piece, as you suggested, rather than applying a single
  large patch.

Let me know your thoughts.

Cheers,
Stefano


On Mon, 10 Feb 2025, Oleksandr Andrushchenko wrote:
> Hello, everybody!
> 
> What is the rationale behind introducing a tool to help with coding style
> verification? I will partially quote Viktor Mitin here [2]:
> 
> "The Xen Project has a coding standard in place, but like many projects, the
> standard is only enforced through peer review. Such mistakes slip through and
> code is imported from other projects which may not follow the same standard.
> The
> goal would be to come up with a tool that can audit the code base as part of a
> CI loop for code style inconsistencies and potentially provide corrections.
> This
> tool is to embed as a part of the continuous integration loop."
> 
> I would add that it would better reflect reality to say that Xen's coding
> style
> is quite incomplete to become a standard and needs some improvement to achieve
> that.
> 
> Here, I would like to provide a bit of history and acknowledge those brave
> individuals who dared and tried to introduce to Xen coding style checking and
> formatting support with clang-format.
> 
> Year 2017, Ishani Chugh.
> ---------------------------------------------------------------------
> This journey began with a request from an Outreachy program member [1].
> Then came the first patches by Ishani Chugh [2]
> Status: *busted*.
> 
> Year 2019, Viktor Mitin
> ---------------------------------------------------------------------
> Then picked up by Viktor Mitin, EPAM's first attempt [3].
> Status: *busted*.
> 
> Year 2020, Anastasiia Lukianenko
> ---------------------------------------------------------------------
> Continued by Anastasiia Lukianenko, EPAM's second attempt [4], [5].
> Some contributions were made to LLVM to make clang-format a better fit for
> Xen [6].
> Xen-summit and presentation [7] and the summary document [8].
> Status: *busted*.
> 
> Year 2023, Luca Fancellu
> ---------------------------------------------------------------------
> Luca restarted it, first ARM attempt [9], [10], [11].
> Status: *busted*.
> 
> That's all for now, but it is still impressive as of 2025.
> 
> So, in my opinion, what were the main issues with all these attempts? There
> are
> many different views, but I would emphasize the following:
> 
> 1) clang-format doesn't perfectly fit Xen's code style as some rules it
> applies
> are not liked by the community or it applies rules that Xen's coding style
> doesn't define (those Luca described in his .clang-format for every clang
> option).
> 
> 2) clang-format doesn't work in a "one-option-at-a-time" mode [12]: clang
> maintainers strongly oppose requests to allow turning off all options except
> some. Believe it or not, other maintainers also have strong opinions on what
> is
> right and what is not for their projects, and this is one area where they will
> not compromise.
> 
> 3) The size of the patch after applying clang-format is huge. Really.
> Something
> like 9 MB. Even if everyone agrees that the approach is good and we can
> proceed
> with it, it is highly unlikely anyone will be able to review it. Considering
> that new patches are being added to the upstream during such a review, it may
> also lead to new code style violations or require a new review of that huge
> patch.
> 
> 4) Which clang-format version should we set as the one used by Xen, so it is
> easy for everyone to use it on their hosts?
> 
> 5) You name it. I think many people in the community can name their points for
> and against clang-format.
> 
> So, in this attempt, I would suggest the following approach:
> I think that I could start sending patches after the latest .clang-format 21
> for
> some part of Xen, ARM code base for example, using work already done by Luca.
> This way:
> 
> 1) Patches are formatted with clang-format, which is a strong plus.
> 2) The diff is well maintained and I can still alter it by hand if there are
> comments or dislikes.
> 3) Finally, when the patch is accepted, we can be more confident that
> clang-format will find far fewer inconsistencies than if it were just applied
> to
> the "raw" code. Thus, the next time clang-format runs, it will produce a much
> smaller patch than before.
> 4) Finally, introduce clang-format and run it on the leftovers: at this stage,
> it would be much easier to discuss every option clang has and the resulting
> output it produces.
> 5) Update existing/add new rules to the Xen coding style based on community
> agreements and the results of this activity.
> 
> We may define the subsystems to start this activity on and also define an
> acceptable size of the patch for review, say 100K. Considering that the longer
> the review, the more outdated the patch becomes and will require a new round
> as
> new code comes in.
> 
> I would love to hear from the community on this approach and finally get it
> done. Not busted.
> 
> Stay safe,
> Oleksandr Andrushchenko
> 
> [1]
> https://lore.kernel.org/xen-devel/1130763396.5603480.1492372859631.JavaMail.zimbra@research.iiit.ac.in/T/#m1db2521362edd286875acf10296873993226dcf2
> [2] https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01739.html
> [3] https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01862.html
> [4] https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg02157.html
> [5] https://lists.xenproject.org/archives/html/xen-devel/2020-10/msg00022.html
> [6] https://reviews.llvm.org/D91950
> [7]
> https://xenproject.org/blog/clang-format-for-xen-coding-style-checking-scheduled/
> [8]
> https://docs.google.com/document/d/1MDzYkPgfVpI_UuO_3NRXsRLAXqIZ6pj2btF7vsMYj8o
> [9] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02294.html
> [10]
> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg00498.html
> [11]
> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01993.html
> [12] https://github.com/llvm/llvm-project/issues/54137#issuecomment-1058564570
> 

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Tue, Feb 11, 2025 at 02:33:08PM -0800, Stefano Stabellini wrote:
> Hi Oleksandr,
> 
> This morning, we had a discussion among maintainers, and the suggested
> approach moving forward is as follows:
> 
> - First, it would be helpful to see a sample of the proposed changes
>   applied to a single source file as an example. If you could provide
>   such a patch, it would help advance the discussion.
> 
> - If the changes are acceptable, we need to properly document the new
>   coding style in xen.git. If not, we will need to iterate again. We may
>   also need to add a "xen" template to clang-format.
> 
> - Once finalized, we will proceed by making changes to the Xen source
>   code piece by piece, as you suggested, rather than applying a single
>   large patch.

No objections, just wandering myself whether it was considered to
initially only apply the new style to new chunks of code?  Using
`git-clang-format` or similar as suggested by Anthony.

Is the adjusted style expected to be too different from the current
one as such approach would lead to hard to read code due to the mixed
styles?

Thanks, Roger.

Re: Coding Style Review and Automation

[Grygorii Strashko]

Hi

On 12.02.25 11:14, Roger Pau Monné wrote:
> On Tue, Feb 11, 2025 at 02:33:08PM -0800, Stefano Stabellini wrote:
>> Hi Oleksandr,
>>
>> This morning, we had a discussion among maintainers, and the suggested
>> approach moving forward is as follows:
>>
>> - First, it would be helpful to see a sample of the proposed changes
>>    applied to a single source file as an example. If you could provide
>>    such a patch, it would help advance the discussion.
>>
>> - If the changes are acceptable, we need to properly document the new
>>    coding style in xen.git. If not, we will need to iterate again. We may
>>    also need to add a "xen" template to clang-format.
>>
>> - Once finalized, we will proceed by making changes to the Xen source
>>    code piece by piece, as you suggested, rather than applying a single
>>    large patch.
> 
> No objections, just wandering myself whether it was considered to
> initially only apply the new style to new chunks of code?  Using
> `git-clang-format` or similar as suggested by Anthony.
> 
> Is the adjusted style expected to be too different from the current
> one as such approach would lead to hard to read code due to the mixed
> styles?

Sorry for may be dumb question, but wouldn't it be reasonable to consider
adding just .clang-format specification to the Xen code base without
automation features?

For example, I've downloaded .clang-format from [1] and using it with my editor
which supports clang-format integration. So, I can just select chunk of code and
do auto-format on it. It speed ups my work very much and results make me happy more
than 90% of the times.

So, it would be nice to have it out of the box while cloning Xen code instead
of searching for it, even if it's not perfect for now.

Unhappy: it's probably "known" things - identification of complex defines and
static struct/arrays declarations.

For example original code:
DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU)
     .dt_match = ipmmu_dt_match,
     .init = ipmmu_init,
DT_DEVICE_END

And after auto format (me, personally, unhappy):
DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU)
     .dt_match = ipmmu_dt_match, .init = ipmmu_init,
DT_DEVICE_END

Best regards,
-grygorii

Re: Coding Style Review and Automation

[Roger Pau Monné]

On Wed, Feb 12, 2025 at 01:14:20PM +0200, Grygorii Strashko wrote:
> Hi
> 
> On 12.02.25 11:14, Roger Pau Monné wrote:
> > On Tue, Feb 11, 2025 at 02:33:08PM -0800, Stefano Stabellini wrote:
> > > Hi Oleksandr,
> > > 
> > > This morning, we had a discussion among maintainers, and the suggested
> > > approach moving forward is as follows:
> > > 
> > > - First, it would be helpful to see a sample of the proposed changes
> > >    applied to a single source file as an example. If you could provide
> > >    such a patch, it would help advance the discussion.
> > > 
> > > - If the changes are acceptable, we need to properly document the new
> > >    coding style in xen.git. If not, we will need to iterate again. We may
> > >    also need to add a "xen" template to clang-format.
> > > 
> > > - Once finalized, we will proceed by making changes to the Xen source
> > >    code piece by piece, as you suggested, rather than applying a single
> > >    large patch.
> > 
> > No objections, just wandering myself whether it was considered to
> > initially only apply the new style to new chunks of code?  Using
> > `git-clang-format` or similar as suggested by Anthony.
> > 
> > Is the adjusted style expected to be too different from the current
> > one as such approach would lead to hard to read code due to the mixed
> > styles?
> 
> Sorry for may be dumb question, but wouldn't it be reasonable to consider
> adding just .clang-format specification to the Xen code base without
> automation features?

Yes, ti can be considered, but I think part of the desire to have
clang-format is so checking can be automated.

In any case, even if checking is not initially automated, whatever
rules are in .clang-format must be in-line with the coding style
document.  Otherwise the presence of .clang-format would just be
misleading if the resulting generated format doesn't adhere to our
coding style.

Thanks, Roger.

Re: Coding Style Review and Automation

[Edwin Torok]

On 11 Feb 2025, at 18:54, Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> wrote:
> 
> On Tue, Feb 11, 2025 at 03:06:08PM +0100, Roger Pau Monné wrote:
>> On Tue, Feb 11, 2025 at 11:19:23AM +0100, Jan Beulich wrote:
>>> On 11.02.2025 10:10, Luca Fancellu wrote:
>>>>>> 3) The size of the patch after applying clang-format is huge. Really. Something
>>>>>> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
>>>>>> with it, it is highly unlikely anyone will be able to review it. Considering
>>>>>> that new patches are being added to the upstream during such a review, it may
>>>>>> also lead to new code style violations or require a new review of that huge
>>>>>> patch.
>>>>> 
>>>>> I think this approach is difficult.  It would likely introduce a lot
>>>>> of noise when using `git blame` (I know, it's just one extra jump,
>>>>> but...), plus would likely break every patch that we currently have
>>>>> in-flight.
>>>> 
>>>> I think we already discussed this in the past and having some churn was accepted,
>>>> also about breaking existing patches, every change merged has the potential to do
>>>> that, this one is more likely but it’s the game I guess?
>>> 
>>> That's easy to say if you have just a few patches in flight, yet I'm worried
>>> about this when considering the hundreds of mine that are awaiting review.
>> 
>> There are also downstreams (including distros) with varying length of
>> patch queues on top of Xen.  Arguably they have to rebase the queue
>> every time they update, but a wide change in coding style will likely
>> be fairly disruptive to them.
>> 
>> Don't take this as a reason to reject clang-format.  As mentioned
>> elsewhere I think the format supported by clang-format would need to
>> be fairly similar to the current Xen one (up to the point that chunks
>> of code using the new and the old style could live together).  Then we
>> would enforce it only for newly added chunks of code initially IMO.
> 
> While clang-format surely will force some changes (the earlier 9MB patch
> is a data point here...)

Here is an example of how reformatting the OCaml C stubs would look like based on an earlier attempt:
https://github.com/edwintorok/xen/commit/7ad754fcfb490954f7f01f788893f5c4b77fdc9a
https://github.com/edwintorok/xen/commit/41155c78cc95fd66fe2ed0d1634b0e59fcc3a3b2

In this case switching those files to Linux coding style results in a smaller diff, and might help reduce that 9MB a little bit.
Those files didn’t follow either the Xen or Linux coding style previously, seems to have been a mix of styles,
which is very confusing because it is not documented anywhere what style they are supposed to be, so everyone is left guessing as to
how to best preserve the style of existing code.


> , I generally expect it should match fairly
> close to the current code style, and so the rebase shouldn't be _that_
> painful. In some cases git likely will handle large part of the work
> already.


There are a few tools that might help with this.

This would reformat only diffs/commits:
https://github.com/llvm/llvm-project/blob/main/clang/tools/clang-format/git-clang-format

These are git merge drivers for clang-format that can help automatically solve most conflicts that a patch rebase would create:
https://github.com/llvm/llvm-project/blob/c174cc48401292e2eb9317128f56fd22af2f4848/libcxx/utils/clang-format-merge-driver.sh#L4
https://github.com/emilio/clang-format-merge
As you can see the implementation is very simple: you use clang-format on all 3 sides during a conflict, and then attempt to merge again. If that has solved the conflict there is nothing more to do, if it hasn’t you get the usual conflict markers to solve it, but this time on the reformatted code.

If you are looking for something more automated to handle your work-in-progress patches, then there are 2 other tools that can help:

Jujutsu’s ‘fix’ command which goes through all (or subset) of your local and mutable commits (not part of upstream repo) and reformats each change individually (which is useful if your base is already well-formatted, and you’ve made a lot of changes to split/reorder/rebase your patches and didn’t use clang-format during development): https://jj-vcs.github.io/jj/latest/
(JJ can be used in ‘git’ colocation/compatibility mode, and I’ve actually switched to using it on a daily basis, because it enables a mega-merge workflow, where you can create a merge commit of all your in-flight branches that gets automatically rebased whenever one of your branches gets rebased, and with jj’s way of deferring conflict resolution you don’t even need to immediately fix the conflicts, which is very useful for “parallelising” patches, i.e. splitting out commits that are independent.
And ‘jj fix’ can keep up with formatting changes during that workflow too)

If JJ isn’t your cup of tea, then there are other tools that can help too, e.g. ‘git-branchless’ has a ‘git test run’ command that can be used to run a formatter on all commits, see https://github.com/arxanas/git-branchless/discussions/803 (using ‘rustfmt’ as an example, but it should work in similarly with ‘clang-format’)

Also if you are worried about git blame then you can create a list of commits to ignore that were due to formatting, here is an example of how that looked like in the XAPI project:
https://github.com/xapi-project/xen-api/blob/master/.git-blame-ignore-revs

We went through a similar process in the XAPI project when we introduced `ocamlformat`, and had similar concerns over back-portability / in-flight work / etc.
But at least for me the equivalent to the above tools for ‘ocamlformat’ made it a fairly smooth process (https://ocaml.org/p/merge-fmt/0.3), and even if the coding style may not suit everyone’s tastes, it is at least *consistent*,
 which is more important than what the coding style actually is (e.g. it prevents many mistakes where incorrect indentation or understanding of precedence leads to incorrect code that looks deceptively correct, but when run through the formatter the bug is immediately obvious)
It also allows the reviews to focus on the contents of the change, rather than nitpicking on style.

Reviewing the initial changes can indeed be difficult, but if the patch submitter specifies the exact version and command they used to create the re-formatting commit in the commit message itself, then reviewers can rerun that command on the parent commit,
and verify that they get the same (or equivalent) outcome.
(This requires the tool to be deterministic of course and always produce the same output given the same inputs).



Best regards,
—Edwin

> 
> It surely is a cost of introducing such a change, but IMO it's a cost
> worth paying.
> 
> -- 
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab

Re: Coding Style Review and Automation

[Oleksandr Andrushchenko]

Hello, everybody!

On 12.02.25 00:33, Stefano Stabellini wrote:
> Hi Oleksandr,
>
> This morning, we had a discussion among maintainers, and the suggested
> approach moving forward is as follows:
Great news!!
>
> - First, it would be helpful to see a sample of the proposed changes
>    applied to a single source file as an example. If you could provide
>    such a patch, it would help advance the discussion.
Sure, I can create such a patch. I will quote Jan here:
"May I suggest to consider e.g. drivers/ instead? That'll involve
more people (to build an opinion) while at the same time it's fewer files
and less code overall."

Which seems to perfectly fit this exercise. So, I'll run clang-format on drivers
and see what file is most affected and then make a patch out of that.
For that I am going to use the latest clang-format (main branch), so we can see
if there is some progress after what Luca sent before (aka 9MB patch) [1].

New clang-format already has some improvements made by Anastasiia Lukianenko
back in 2020 and those have landed the upstream, for example [2].
Those were specifically made to better suit Xen coding style.

Luca was targeting the clang-format which is widely available (version 15) and
thus was not able to benefit from those: but nevertheless, I will prepare the
patch with Anastasiia's improvements plus the Luca's .clang-format: I'll merge
.clang-format from both authors.

For the reference: I've put Luca's .clang-format into my Xen fork for now along
with gathered feedback from the community on some clang-format options and
their settings [3] as those were discussed on the mailing list back in 2023.
I will re-work that in order to add changes made by Anastasiia.

>
> - If the changes are acceptable, we need to properly document the new
>    coding style in xen.git. If not, we will need to iterate again. We may
>    also need to add a "xen" template to clang-format.
I am not sure that single patch is going to show all the changes and we'll be
able to make the right decision out of that. That was already proven by the size
of the patch Luca posted: 9M. But, anyways, this will definitely show what can
be accepted now.

>
> - Once finalized, we will proceed by making changes to the Xen source
>    code piece by piece, as you suggested, rather than applying a single
>    large patch.
>
> Let me know your thoughts.
Sounds good, thank you!!

Stay safe,
Oleksandr Andrushchenko

[1] https://gitlab.com/luca.fancellu/xen-clang/-/commit/8938bf2196be66b05693a48752ebbdf363e8d8e1.patch
[2] https://github.com/llvm/llvm-project/commit/f6b252978c40bc437d8495218a69e3bd166b105b
[3] https://github.com/andr2000/xen/blob/clang/xen/.clang-format
>
> Cheers,
> Stefano
>
>
> On Mon, 10 Feb 2025, Oleksandr Andrushchenko wrote:
>> Hello, everybody!
>>
>> What is the rationale behind introducing a tool to help with coding style
>> verification? I will partially quote Viktor Mitin here [2]:
>>
>> "The Xen Project has a coding standard in place, but like many projects, the
>> standard is only enforced through peer review. Such mistakes slip through and
>> code is imported from other projects which may not follow the same standard.
>> The
>> goal would be to come up with a tool that can audit the code base as part of a
>> CI loop for code style inconsistencies and potentially provide corrections.
>> This
>> tool is to embed as a part of the continuous integration loop."
>>
>> I would add that it would better reflect reality to say that Xen's coding
>> style
>> is quite incomplete to become a standard and needs some improvement to achieve
>> that.
>>
>> Here, I would like to provide a bit of history and acknowledge those brave
>> individuals who dared and tried to introduce to Xen coding style checking and
>> formatting support with clang-format.
>>
>> Year 2017, Ishani Chugh.
>> ---------------------------------------------------------------------
>> This journey began with a request from an Outreachy program member [1].
>> Then came the first patches by Ishani Chugh [2]
>> Status: *busted*.
>>
>> Year 2019, Viktor Mitin
>> ---------------------------------------------------------------------
>> Then picked up by Viktor Mitin, EPAM's first attempt [3].
>> Status: *busted*.
>>
>> Year 2020, Anastasiia Lukianenko
>> ---------------------------------------------------------------------
>> Continued by Anastasiia Lukianenko, EPAM's second attempt [4], [5].
>> Some contributions were made to LLVM to make clang-format a better fit for
>> Xen [6].
>> Xen-summit and presentation [7] and the summary document [8].
>> Status: *busted*.
>>
>> Year 2023, Luca Fancellu
>> ---------------------------------------------------------------------
>> Luca restarted it, first ARM attempt [9], [10], [11].
>> Status: *busted*.
>>
>> That's all for now, but it is still impressive as of 2025.
>>
>> So, in my opinion, what were the main issues with all these attempts? There
>> are
>> many different views, but I would emphasize the following:
>>
>> 1) clang-format doesn't perfectly fit Xen's code style as some rules it
>> applies
>> are not liked by the community or it applies rules that Xen's coding style
>> doesn't define (those Luca described in his .clang-format for every clang
>> option).
>>
>> 2) clang-format doesn't work in a "one-option-at-a-time" mode [12]: clang
>> maintainers strongly oppose requests to allow turning off all options except
>> some. Believe it or not, other maintainers also have strong opinions on what
>> is
>> right and what is not for their projects, and this is one area where they will
>> not compromise.
>>
>> 3) The size of the patch after applying clang-format is huge. Really.
>> Something
>> like 9 MB. Even if everyone agrees that the approach is good and we can
>> proceed
>> with it, it is highly unlikely anyone will be able to review it. Considering
>> that new patches are being added to the upstream during such a review, it may
>> also lead to new code style violations or require a new review of that huge
>> patch.
>>
>> 4) Which clang-format version should we set as the one used by Xen, so it is
>> easy for everyone to use it on their hosts?
>>
>> 5) You name it. I think many people in the community can name their points for
>> and against clang-format.
>>
>> So, in this attempt, I would suggest the following approach:
>> I think that I could start sending patches after the latest .clang-format 21
>> for
>> some part of Xen, ARM code base for example, using work already done by Luca.
>> This way:
>>
>> 1) Patches are formatted with clang-format, which is a strong plus.
>> 2) The diff is well maintained and I can still alter it by hand if there are
>> comments or dislikes.
>> 3) Finally, when the patch is accepted, we can be more confident that
>> clang-format will find far fewer inconsistencies than if it were just applied
>> to
>> the "raw" code. Thus, the next time clang-format runs, it will produce a much
>> smaller patch than before.
>> 4) Finally, introduce clang-format and run it on the leftovers: at this stage,
>> it would be much easier to discuss every option clang has and the resulting
>> output it produces.
>> 5) Update existing/add new rules to the Xen coding style based on community
>> agreements and the results of this activity.
>>
>> We may define the subsystems to start this activity on and also define an
>> acceptable size of the patch for review, say 100K. Considering that the longer
>> the review, the more outdated the patch becomes and will require a new round
>> as
>> new code comes in.
>>
>> I would love to hear from the community on this approach and finally get it
>> done. Not busted.
>>
>> Stay safe,
>> Oleksandr Andrushchenko
>>
>> [1]
>> https://lore.kernel.org/xen-devel/1130763396.5603480.1492372859631.JavaMail.zimbra@research.iiit.ac.in/T/#m1db2521362edd286875acf10296873993226dcf2
>> [2] https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01739.html
>> [3] https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01862.html
>> [4] https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg02157.html
>> [5] https://lists.xenproject.org/archives/html/xen-devel/2020-10/msg00022.html
>> [6] https://reviews.llvm.org/D91950
>> [7]
>> https://xenproject.org/blog/clang-format-for-xen-coding-style-checking-scheduled/
>> [8]
>> https://docs.google.com/document/d/1MDzYkPgfVpI_UuO_3NRXsRLAXqIZ6pj2btF7vsMYj8o
>> [9] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02294.html
>> [10]
>> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg00498.html
>> [11]
>> https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg01993.html
>> [12] https://github.com/llvm/llvm-project/issues/54137#issuecomment-1058564570
>>

Re: Coding Style Review and Automation

[Oleksandr Andrushchenko]

Hi, Grygorii,

On 12.02.25 13:14, Grygorii Strashko wrote:
> Hi
>
> On 12.02.25 11:14, Roger Pau Monné wrote:
>> On Tue, Feb 11, 2025 at 02:33:08PM -0800, Stefano Stabellini wrote:
>>> Hi Oleksandr,
>>>
>>> This morning, we had a discussion among maintainers, and the suggested
>>> approach moving forward is as follows:
>>>
>>> - First, it would be helpful to see a sample of the proposed changes
>>>    applied to a single source file as an example. If you could provide
>>>    such a patch, it would help advance the discussion.
>>>
>>> - If the changes are acceptable, we need to properly document the new
>>>    coding style in xen.git. If not, we will need to iterate again. 
>>> We may
>>>    also need to add a "xen" template to clang-format.
>>>
>>> - Once finalized, we will proceed by making changes to the Xen source
>>>    code piece by piece, as you suggested, rather than applying a single
>>>    large patch.
>>
>> No objections, just wandering myself whether it was considered to
>> initially only apply the new style to new chunks of code?  Using
>> `git-clang-format` or similar as suggested by Anthony.
>>
>> Is the adjusted style expected to be too different from the current
>> one as such approach would lead to hard to read code due to the mixed
>> styles?
>
> Sorry for may be dumb question, but wouldn't it be reasonable to consider
> adding just .clang-format specification to the Xen code base without
> automation features?
>
> For example, I've downloaded .clang-format from [1] and using it with 
> my editor
> which supports clang-format integration. So, I can just select chunk 
> of code and
> do auto-format on it. It speed ups my work very much and results make 
> me happy more
> than 90% of the times.
>
> So, it would be nice to have it out of the box while cloning Xen code 
> instead
> of searching for it, even if it's not perfect for now.
>
> Unhappy: it's probably "known" things - identification of complex 
> defines and
> static struct/arrays declarations.
>
> For example original code:
> DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU)
>     .dt_match = ipmmu_dt_match,
>     .init = ipmmu_init,
> DT_DEVICE_END
>
> And after auto format (me, personally, unhappy):
> DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU)
>     .dt_match = ipmmu_dt_match, .init = ipmmu_init,
> DT_DEVICE_END
>
This is covered by [1]
> Best regards,
> -grygorii
[1] https://github.com/andr2000/xen/blob/clang/xen/.clang-format#L859