Even in those cases where there is some particular manual formatting I think we should strongly consider ditching it in favor of the auto-formatted version. In the .td example with the vertical alignment I don’t think one version is objectively more readable than the other to begin with.
1 Like
Thanks for those pointers! I spent some time trying to understand how the pieces all fit together, and I’m not sure this is really a good solution in the long term.
For the libcxx reformat, the project-wide .gitattributes file gained entries for the libcxx project files. (Probably this should have been put in libcxx/.gitattributes?)
AFAICT (without actually trying it) this means that every merge in libcxx is running this special merge script, i.e. it’s running clang-format 3 times per merged file–whether the file has already been caught up or not. Am I reading that correctly? (Also, you have to have the correct version of clang-format on PATH…)
I’d kind of rather tell people to “go here to get the right clang-format, then run it on your local repo, then you can merge the upstream reformat commit.” Of course that can be packaged more conveniently, but you get the idea.
While making sure the code base is formatted is nice, I think the most frequent annoyance for me are files with trailing whitespaces and missing lines at end of file.
It would be nice to fix them all, and to be able to tool that (including in doc files, test directories etc)
Here’s a PR reformatting all of lld. I did all the files, not just small/medium, so people can get an idea of what that looks like.
Here’s a PR with a helper script that downstream users can run to resolve conflicts from a reformatting merge. It is very close to the libcxx script, but implemented for explicitly running via git mergetool rather than automatically running on every merge. The idea is that if you get a conflict after merging a reformat, you run git mergetool and it will fix the conflicts for you. It takes an extra manual step, but doesn’t impose a cost on every merge forever.
1 Like
Another thing: github pull requests automatically run clang-format tests on every pull request, and if the test doesn’t pass, you can’t commit the pull request through the github interface. This is true even if none of the parts of the file that fail clang-format were updated by the pull request.
Yet another consideration: Apparently github uses git-clang-format for these tests, which differs slightly from clang-format.
This sounds wrong. You should definitely be able to ignore the formatting and merge anyway. And if you see PRs where “the wrong code” was pointed out or formatted I would appreciate if you open an issue and we will have a look, since that would be a big.
1 Like
Here’s a recent one where I intentionally ignored what the clang-format bot was asking me to do, since it would have been unnecessarily disruptive to downstream maintainers: [llvm][unittests] Put human-readable names on TargetParserTests. NFC by jroelofs · Pull Request #80749 · llvm/llvm-project · GitHub
@boomanaiden154-1 any ideas why it reformatted that much here?
Another thing: github pull requests automatically run clang-format tests on every pull request, and if the test doesn’t pass, you can’t commit the pull request through the github interface.
I’m not sure this is accurate: you should be able to override branch protection rules in our repo.
This is true even if none of the parts of the file that fail clang-format were updated by the pull request.
I’m also fairly confident that this isn’t right either: the pre-merge check should only be checking the parts of the file you modified. See RFC: clang-format github action - #6 by pogo59.
I suspect the reason is that patch touches a long comma-separated list of things.
Is there a reason why formatting job on BuildKite is still running and reporting formatting issues in files entirely unrelated to PR? GitHub Actions job seems to work fine.
For reference, when LLD went through its mass renaming (which I merged into our downstream ~5(?) years ago), this was the approach taken. You took the magic renaming script, ran it on your tree, and used that as the result of the merge. Though admittedly LLD is orders of magnitude smaller than other LLVM components; small enough that you could glance over the whole diff.
1 Like