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.
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!
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.
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::
tostd::
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.