[RFC] Clang-formatting all of libc++ once and for all

[ldionne]

@Endill

I have some conflicting feelings about “once and for all” part of the title, because clang-format and hard-coded LLVM style are evolving, too, and you haven’t touched on how libc++ is going to deal with it.

@cjdb

We should also adopt a policy on how often we want to run clang-format project-wide. […]

Those are good points. First, we would pin down the canonical clang-format version to be the same as the one used in the clang-format CI job here. Currently that is clang-format 17.0.1. This way, our code base as a whole would be consistent with the version of clang-format we’re using to enforce formatting in the CI, which is important to avoid confusion.

Then, as @jrtc27 suggested, I think we could update this Clang-format version once per release cycle and reformat all of libc++ with it. I do expect that most code would remain unchanged so doing that would probably be pretty smooth. If we notice a more major change that would be really disruptive, I think we could just handle when/if it happens.

@Endill

If it hasn’t been done already, setting up a regression test for clang-format seems useful to prevent clang-format changes to make formatting worse (or at least make it visible before said changes are merged).

The monorepo as a whole does have a clang-format CI job that ensures that all newly checked-in code conforms to clang-format, so I think we’re good on that point!

@cjdb

Does there need to be a discussion about the contents of our format style, or are we rolling with what’s already there?

I would suggest going with what we have. Large parts of our code are already formatted with that style, it’s been working pretty well and I haven’t seen many complaints about it. There were some concepts-related issues initially but those got resolved in recent clang-format versions. So basically, since nobody’s complaining I think we should just keep what we have.

@cjdb

However, I will caution against doing this right now, as there are certain bugs in clang-format that we’ll need to deal with because they hurt readability quite a lot. Perhaps it’s worth prioritising addressing these bugs as a matter of libc++?

I am not aware of any serious bugs in clang-format that have been causing issues with our formatting. Or perhaps there are but we have just learned to live with them In all cases, we are enforcing clang-format for new code regardless, so I think we should definitely avoid blocking this reformatting on clang-format bugs. Instead we should just pick up fixes for said bugs in future re-formattings and use clang-format off/on in places that really don’t make sense for now.

---

Overall, it looks like people are quite favorable to the idea presented here. I am going to wait a few days and then follow this timeline:

• This week: Propose an optional change to also rename _LIBCPP_INLINE_VISIBILITY to _LIBCPP_HIDE_FROM_ABI and _VSTD:: to std:: prior to the formatting change, since we could kill two birds with one stone. This is optional and I don’t want to necessarily couple the two changes, but I think that this additional change is simple enough that it should be pretty uncontroversial.
• This week: Put out for review the merge driver script that would allow transparently rebasing across clang-format changes. PR HERE
• Dec 4th: Open a PR that clang-formats the whole code base. We will iterate on that PR, extracting bits where we need clang-format off/on out of the PR until we’re happy.
• Dec 11th (goal): Land the clang-format patch assuming we’re ready. This date might slip depending on what we find in the previous step.

This is a fairly aggressive timeline, however in practice there’s not much technical work remaining and the more we wait, the longer we have to deal with creeping clang-format changes in PRs, so I think it makes sense to avoid delaying this more than needed.