|
From: Nicholas N. <n.n...@gm...> - 2023-04-27 00:06:25
|
On Thu, 27 Apr 2023 at 00:04, Bart Van Assche <bva...@ac...> wrote: > Are these the only disadvantages of reformatting the entire code base? I > see more disadvantages: > > - Preserving the formatting of code that is in tabular format requires > manual annotation (/* clang-format off */ and /* clang-format on */). Who > will do the tedious work of reviewing the entire code base and annotating > all the code for which formatting should be preserved? > > I'm happy to start. I think doing the work in chunks makes sense, e.g. one subdirectory at a time, something like that. After that, others can contribute if they like. A prerequisite for that is deciding what rules to put in the .clang-format file. I am also happy to work on that. Some discussion, along with example diffs, will be necessary. > > - It is a certainty that reformatting the code with clang-format will > make formatting worse for a significant fraction of the code base. A > summary of the formatting made worse by clang-format for one particular > Linux kernel source file is available here: *Re: [PATCH] scsi: > megaraid: cleanup formatting of megaraid > <https://lore.kernel.org/linux-scsi/CAKwvOdnWHVV+3s8SO=Q8F...@ma.../>* > . > > That's a handful of quibbles. There will always be places where an auto-formatter does something that somebody doesn't like. That is also true when humans format code. > > - It is likely that the Valgrind .clang-format file will be modified > in the future. The Linux kernel .clang-format style file was introduced in > 2018 and since then has been modified 44 times: > > $ git log .clang-format | grep -c ^commit > 45 > > Will the entire Valgrind code base be reformatted every time the > Valgrind .clang-format file is modified? > > I expect .clang-format changes to be rare and minimal, and some care will be needed when deciding what rules to put in the initial .clang-format file. Changing the indent size every six months would be silly, of course. But to answer the question: absolutely. Because the goal here is to make auto-formatting mandatory. Any code that isn't formatted properly would be rejected. So if the style changes, the code changes with it. (Mandatory pre-commit testing, another modern software development convenience that Valgrind currently lacks, would help with this.) As for the Linux example: the "modified 44 times" is highly misleading. The version history of the file can be seen here <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/.clang-format>. The file currently has 688 lines, but 81% of those lines are for macros listed in the `ForEachMacros <https://clang.llvm.org/docs/ClangFormatStyleOptions.html#foreachmacros>` rule, which is a list of macros that needs particular treatment. Most of the 44 modifications are just modifications of that list, which are trivial and won't affect any code other than those macros, and aren't relevant for Valgrind. There were only five commits with meaningful changes that didn't involve that list: one, <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=c90f3b8c4b5b7ff3ee7103640a2ab1c960c69645> two <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=c90f3b8c4b5b7ff3ee7103640a2ab1c960c69645>, three <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=96232c7d4f847a5e597177236159e6b32ccf60e4>, four <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=d7f6604341c748f803810664d5603af22b84a8cc>, five <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=781121a7f6d11d7cae44982f174ea82adeec7db0>. Only one of those (the third one) changes more than one rule, and that was part of the upgrade from clang-format 4 to 6. clang-format is more mature now so changes of that sort are less likely to be needed in the future. > > > Is the above list complete? Did I perhaps overlook any disadvantages? > Auto-formatting is very common these days, to the point of being standard, and blanket opposition to auto-formatting has become a minority position. Which makes sense, because many projects have style guides, and automatic enforcement of style is better than manual enforcement, being less work and more reliable. Valgrind's development processes need to move with the times. Nick |