RFC: clang-format all the things!

LLVM Project
21

The only case I have in mind is the obvious “compressed switch / list” situation where we can fit the case+return/call+break all on a single line.

22

To work with patches from before the reformatting, the solution is indeed to format all versions before doing a (3-way) merge. For the libcxx reformat, @ldionne added a git merge driver that can help with this, and that applies this strategy.

1 Like
23

Re. pr-code-format finding clang-format: What is “aminya/setup-cpp@v1” ? It looks kinda like a branch, but “git branch -l -r” doesn’t find anything. (Trying to understand where the clang-format executable comes from.)

24

I’m generally in support here, but details matter.

My preference here would be something along the following:

  • Create a list of clang-format clean files, check it in, adjust the github bot to scream loudly about to format breakages in those files only. Removing a file from this list is subject to normal review - nothing higher, nothing lower.
  • On a per sub-project basis, encourage clang-format only changes (subject to normal review requirements) which do not introduce “odd” formatting or require the use of the pragma disable. Discourage “large” per-file changes at this time. Once done, update the clang-format file list above.
  • Let the above settle. Others are discussing the timing point here, so I won’t bother. I also liked the gitignore file suggestion - we should document that.
  • Revisit this discussion once we have more visibility on what the tail looks like. We may decide the work to handle the tail is worthwhile - maybe we fix a couple cases in clang-format - or maybe we don’t. If 80-90% of the codebase is machine formatted, and we can remove a file (subject to normal review) with cause, I think that’s a reasonable end state.
25

Looks like a tag of this actions extension GitHub - aminya/setup-cpp: Install all the tools required for building and testing C++/C projects..

26

There’s actually quite a few places in clang we intentionally ignore clang-format results (but don’t use the comments unfortunately), so I’d be against doing this fully mechanically. I can think of a few places we do it where enums have initializers that we want to keep ordered.

I LIKE the idea of being fully clang-format-ized, but I’d want us to spend a significant amount of time looking through the diffs to make sure we go back and mark the locations we want to keep.

Doing it ‘small piece at a time’ and going through normal review seems to me to be a good idea, that would give us a chance to get clang-format off consistent.

1 Like
27

Given the concern, I could support this approach.

28

Thanks for the pointer @DavidSpickett so it looks like the action has to retrieve a released version? Which we could update occasionally as new releases occur that have desirable changes. In the meantime we’d potentially be adding workarounds in the form of clang-format on/off directives in places where the tool should be doing something different; ideally some of those could be removed after a tool upgrade. I know we’ve done things like that for working around MSVC issues.

The upside is that there’s a known stable tool that the GH action uses, and going forward we can base decisions on that.

29

I’m supportive as well. As another data point, we faced some of the same challenges (albeit on a smaller scale) as part of the Python reformatting and I thought it was a success. LLDB had a lot of non-compliant Python code, a bunch of it downstream, and with clear instructions in the commit message we didn’t see any issues.

30

One of the downsides to this approach is that then we’re going to end up with lots and lots of clang-format commits across the repository. If we instead just bulk reformat the whole project at once, we end up with one commit. The advantage of one commit is that we have one commit across the whole project that we can use as a revision to ignore for git-blame’s --ignore-revs flag.

Conversely we could amend your proposal to also include maintaining an ever growing ignore-revs-file, but I’d really rather we just pull the bandaid off.

I’ve worked on several projects that have done bulk reformatting, and I have generally found it to be mostly painless and vastly preferable to have consistent code formatting. The HLSL compiler recently did a bulk reformatting of all source files not directly inherited from LLVM/Clang. The reformatting has made it so much easier to read code, and I’m a huge fan of not needing to think about formatting because the tool will just do it for me.

4 Likes
31

I spoke with the folks who handle merging community changes in our downstream at Intel and we’re in support of formatting all the files in one chunk and keeping them formatted via precommit CI. We’ll get some merge conflicts from it up front, but we have ways to resolve those. However, we would appreciate agreeing on a fixed version of clang-format so that we’re certain we’re using the same version of clang-format as upstream in order to avoid conflicts from differing versions of the tool. We can upgrade the version of clang-format the same way we handle any other dependency upgrades.

7 Likes
32

I share this concern; we’ve traditionally avoided adding clang-format comments to the Clang code base because of the amount of clutter they add, but we definitely have places we don’t want the clang-format behavior. We’ll have to start using those markings, but that means we can’t really do a mechanical change.

33

The way that we handled this when I made this change for DXC was that I posted a PR with the results of the mechanical re-format then we reviewed it to find the places where we didn’t like the results and we added the comments during review passes.

This didn’t actually prove too onerous. I think the final change that was merged had a few places people were not happy with the formatting, but we just fixed those up after the fact.

34

Yeah, the tricky part for Clang is how many lines are being formatted: 147,145 lines across 1525 files. So long as we split that up into some reasonable chunks, I’m comfortable with that approach. But nobody is going to look at 1500+ files in a single PR.

35

Re. tool versioning: It appears that the CI has to use a released version, currently set to 17.0.1. I haven’t been using that version for my experiments but precise numbers aren’t the point, it’s the scale of the thing that matters right now, and I’m confident that the more-or-less-HEAD version I’m using isn’t different enough to make that data trash.

When it comes time to start updating files in the repo, then the specific version will matter.

If we do this in stages, early stages might discover things that need to be addressed in the tool. If we do that efficiently, we can still get fixes into 18.1.x and then pin the CI to that version going forward. Yes, there’s a chance this will mean reformatting some number of files, but I think we can live with that number being kinda small, and not require it to be zero.

36

What I’m reading so far is that nobody is disagreeing in principle. But there are some varying viewpoints and concerns about how to proceed.

While there are advantages to a big-bang approach, mainly that git-blame can be asked to ignore certain revisions, there are a variety of downsides, and several people wanting to use a more iterative approach.

Given that the tool has some known issues, and there are bits of code randomly throughout the code base that are formatted a particular way and people want to keep those intact, it seems like putting format changes through review is appropriate. For smaller projects, these still might be feasible to do all at once.

I would actually argue in favor of a mixed approach, informed by several of the responses.

  1. Do the “small” and probably “medium” files first, plus “large” files in small projects. This likely wants to be done per project, and for projects with lots of “small” files, probably per library. Because these are small changes (I set an arbitrary cutoff of 20 lines), IMO they aren’t worth adding to the ignore-list. This will take care of > 50% of the non-compliant files.
  2. This experience may surface some things that should be fed back to the tool as bugs or feature requests. We could wait on fixes, if we think they’ll make it into the next release, then update the CI to point to that release. Typical release schedules give us a couple of months for this kind of iteration.
  3. Iteratively review the remaining projects, ideally in ways that don’t burn out the reviewers. I’d expect that project list to be: clang, compiler-rt, lldb, llvm, polly, based on the “Large” column of the table. Note, this is a review-only pass, looking for places that really ought to get clang-format on/off directives.
  4. Big-bang the remaining projects. The commit(s) can be added to the git-blame-ignore list.
  5. Go party on a Mediterranean beach.

Because the review step will take significant time, I’d like to get started with the small/medium changes soon.

Regarding part of @preames suggestion: We already have a “clean” file (see original post) although I don’t know whether it is maintained automatically or manually. Presumably it would be possible to have the GH action do two passes, one for files in the list and one for files not in the list.

It would also obviously be possible to use that list as a filter, and have CI completely ignore files that aren’t on the list. While that feels a little sketchy, if we are moving towards 100% clean, then at that point the “clean” file goes away entirely. (Until we start wanting to clang-format .td files; CI currently doesn’t look at those at all. But that’s a different RFC.)

37

These comments above contain a lot of tangible consequences that will cost real time and money. The tangible benefits of a mass reformatting, on the other hand, are not at all clear. Yes, it’s policy; yes, consistency is nice. But it’s hard to see how this yields a better product for actual users.

38

I tried a little bit with find . -name *.td | xargs clang-format -i, and I found that blindly apply the formats may break some manually made alignments:

@@ -146,109 +143,109 @@ def Headerfile : BlockCommand<"headerfile"> { let IsHeaderfileCommand = 1; }
 // We don't do any additional semantic analysis for the following
 // BlockCommands.  It might be a good idea to do something extra for them, but
 // for now we model them as plain BlockCommands.
-def Arg        : BlockCommand<"arg">;
-def Attention  : BlockCommand<"attention">;
-def Author     : BlockCommand<"author">;
-def Authors    : BlockCommand<"authors">;
-def Bug        : BlockCommand<"bug">;
-def Copyright  : BlockCommand<"copyright">;
-def Date       : BlockCommand<"date">;
-def Invariant  : BlockCommand<"invariant">;
-def Li         : BlockCommand<"li">;
-def Note       : BlockCommand<"note">;
-def Par        : BlockCommand<"par">;
-def Post       : BlockCommand<"post">;
-def Pre        : BlockCommand<"pre">;
-def Remark     : BlockCommand<"remark">;
-def Remarks    : BlockCommand<"remarks">;
-def Retval     : BlockCommand<"retval"> { let NumArgs = 1; }
-def Sa         : BlockCommand<"sa">;
-def See        : BlockCommand<"see">;
-def Since      : BlockCommand<"since">;
-def Test       : BlockCommand<"test">;
-def Todo       : BlockCommand<"todo">;
-def Version    : BlockCommand<"version">;
-def Warning    : BlockCommand<"warning">;
-def XRefItem   : BlockCommand<"xrefitem"> { let NumArgs = 3; }
+def Arg : BlockCommand<"arg">;
+def Attention : BlockCommand<"attention">;
+def Author : BlockCommand<"author">;
+def Authors : BlockCommand<"authors">;
+def Bug : BlockCommand<"bug">;
+def Copyright : BlockCommand<"copyright">;
+def Date : BlockCommand<"date">;
+def Invariant : BlockCommand<"invariant">;
+def Li : BlockCommand<"li">;
+def Note : BlockCommand<"note">;
+def Par : BlockCommand<"par">;
+def Post : BlockCommand<"post">;
+def Pre : BlockCommand<"pre">;
+def Remark : BlockCommand<"remark">;
+def Remarks : BlockCommand<"remarks">;
+def Retval : BlockCommand<"retval"> { let NumArgs = 1; }
+def Sa : BlockCommand<"sa">;
+def See : BlockCommand<"see">;
+def Since : BlockCommand<"since">;
+def Test : BlockCommand<"test">;
+def Todo : BlockCommand<"todo">;
+def Version : BlockCommand<"version">;
+def Warning : BlockCommand<"warning">;
+def XRefItem : BlockCommand<"xrefitem"> { let NumArgs = 3; }
39

It is a good point that there is by definition no tangible value to the user, which I believe is true of any NFC change.

I think there is also no value in every interaction today that requires thought, time, and effort by developers to:

  • Set up their editor to handle formatting only edited regions, when it should be possible to configure format-whole-file-on-save trivially
  • Manually disable editor auto-formatting when working on particular parts of the codebase where there is some strong interest in manual formatting
  • Split out NFC reformatting changes when there is significant reformatting noise that would make the review more difficult (bonus points for when this isn’t done ahead of the review process, so either the reviewer suffers through it or has to add a round-trip to request and wait for that splitting)

I think the difficult thing is to weigh how much the ongoing distributed cost of these kinds of papercut issues compares with the larger up-front cost of fixing the problem once and never thinking about it again.

2 Likes
40

Formatting .td files is out-of-scope for this RFC, although I see similar things happening in (for example) header files with sequences of #defines with aligned definitions. It’s this sort of behavior that makes people want to review the formatted files before they are committed.