[libcxx-commits] [PATCH] D92229: [libc++] Update clang-format configuration

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: goncharov.
ldionne added a comment.
This revision now requires changes to proceed.

In D92229#2421298 <https://reviews.llvm.org/D92229#2421298>, @thakis wrote:

> This is a mostly solved problem: Git 2.23 added support for ignoring whitespace commits in blame output. You store hashes that should be blame-transparent in a file called `.git-blame-ignore-revs` [...]

I've used that for smaller whitespace-changing patches, however does git do a good job at figuring out the blame correctly when stuff moves around a lot? My concern was that `.git-blame-ignore-revs` wouldn't work well for such a large reflow, however if it does, then I'd say my concerns are mostly alleviated.

> We auto-formatted a big chunk of Chromium's code a while ago. We did this before git 2.23 so we wrote a custom tool that does the same thing (https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html), but with git 2.23+ there's even upstream support for this.

That's large enough to be considered a success story, clearly.

In D92229#2422055 <https://reviews.llvm.org/D92229#2422055>, @curdeius wrote:

> @ldionne, I'll need your help to set this up. Agents need to have git-clang-format installed. I haven't tried but I guess I can't `apt install clang-format` in the buildkite script and it doesn't sound like a great idea anyway.

No, indeed. You'd need to update the Dockerfile and install the utility on the macos builders. I'll do it depending on what Mikhail says below -- it's easier for me since they are sitting a meter away :-).

> And, wouldn't it be possible to reuse the existing machinery in llvm/clang premerge checks that runs git-clang-format? I am afraid it can't be reused because it uses different CI pipeline and different agents...

I guess it may be possible. @goncharov Mikhail, if we decide to enable clang-format for libc++, would it be possible to re-enable the clang-format checks? IIRC, I think I had requested to remove them cause they were always broken.



================
Comment at: libcxx/include/variant:1666
 
-  inline _LIBCPP_INLINE_VISIBILITY
-  result_type operator()(const argument_type&) const _NOEXCEPT {
+  inline _LIBCPP_INLINE_VISIBILITY result_type
+  operator()(const argument_type&) const _NOEXCEPT {
----------------
What controls this? I would *love* to move away from declaring the return type of the function above, since it gets lost in our numerous attributes.


================
Comment at: libcxx/utils/ci/run-buildbot:99
+    clean
+    set -o pipefail
+    echo "+++ Checking formatting"
----------------
I think it would be reasonable to set this for the whole script. WDYT?


================
Comment at: libcxx/utils/ci/run-buildbot:103
+    mkdir -p ${BUILD_DIR}
+    git-clang-format --diff --extensions ',h,hh,hpp,hxx,c,cc,cxx,cpp' HEAD~1 -- libcxx/ | tee ${BUILD_DIR}/clang-format.patch
+    # Check if the diff is empty, fail otherwise.
----------------
We should also include `libcxxabi`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92229/new/

https://reviews.llvm.org/D92229