|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: Coding Style Review and Automation
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@xxxxxxxxxxxxxxxxxxx/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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |