Page MenuHomePhabricator

[libc++] Find a clang-format everybody is happy with
ClosedPublic

Authored by philnik on May 2 2022, 11:07 AM.

Details

Summary

We decided that we want to use clang-format for libc++ but we haven't decided yet how the code should be formatted. We should probably discuss things on discord. This PR is mostly to show how the clang-format would look like and to commit to one once we decided on it. I'll remove the <string> diff when I commit this PR.

Diff Detail

Event Timeline

philnik created this revision.May 2 2022, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 11:07 AM
Herald added a subscriber: arichardson. · View Herald Transcript
philnik requested review of this revision.May 2 2022, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 11:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 426491.May 2 2022, 12:39 PM
  • Try to use IndentRequires

First two questions that come to my mind (we can move to Discord if you prefer):

  1. What do we do about the numerous places in the existing codebase with "fancy" formatting? For example, the aligned typedefs inside string, or things like
static_assert( foo<Bar>); // Notice the extra space
static_assert(!foo<Baz>);

AFAIU, our only two alternatives are to either use clang-format on/off "pragmas" around those blocks, or just let go of the fancy formatting. I strongly doubt clang-format can be taught to emulate that (though would be happy to be wrong on this).

  1. What do we do about concepts/requires clauses? From what I've seen, clang-format doesn't seem to understand them and formats them pretty weirdly. You might want to include another file that contains a lot of concepts into this patch to demonstrate the effects. I think our alternatives are to either disable clang-format around those parts (FWIW, I think this is what MSVC does) or try to patch the tool.
philnik updated this revision to Diff 426526.May 2 2022, 2:53 PM
  • Add a few requires examples

The formatting for requires clauses still isn't perfect, but I think these problems can mostly be worked around by adding parentheses around the clause itself. It isn't perfect but I think it's not much of a readability decrease. It's kind-of changing code for the tool to be happy (which I agree isn't a good thing) but I think in this case it's acceptable. IMO the gains of having actually consistent formatting outweighs the cost of adding parantheses in a few places.

ldionne added inline comments.May 6 2022, 10:18 AM
libcxx/include/__ranges/lazy_split_view.h
407 ↗(On Diff #426526)

I have a strong preference for keeping the { on the same line as the if (same for class and for functions). We don't have a 100% consistent way of doing this right now, but I think that's the precedent we should set going forward.

philnik updated this revision to Diff 427830.May 7 2022, 1:54 AM
  • Use LLVM brace wrapping

@ldionne I think having the braces on their own lines gives a much better visual indication where scopes start. But I guess that's an uphill battle for me which I don't really feel like fighting.

I just went to the formatted files and in general I'm quite happy with it. Some concept related parts still don't look great, but I expect most/all of them can be fixed with extra parens.

I have a preference to stay as close to LLVM style as possible.

libcxx/.clang-format
59

This seems to deviate from LLVM style, why do we want to deviate here?

libcxx/include/__ranges/lazy_split_view.h
81 ↗(On Diff #427830)

This looks quite bad, but I expect it's a clang-format error. Does it become better with this change

93 ↗(On Diff #427830)

it looks like this needs manual intervention, but probably one time only.

Personally I don't see much of a reason to stay close to the LLVM style. We already deviate quite a bit to make the code readable. The libc++ code base is quite different from most of the other LLVM projects. Is there any specific reason you want to stay close to the LLVM style @Mordante?

libcxx/include/__ranges/lazy_split_view.h
81 ↗(On Diff #427830)

No, clang-format still puts the = default on it's own line.

93 ↗(On Diff #427830)

putting the , at the usual place fixes this weird formatting.

ldionne accepted this revision.Mon, Jun 13, 2:54 PM

This is not perfect, but it looks reasonable to me. However, some comments;

  1. Let's keep it soft-fail for a bit until we've gained some hands-on experience with formatting our code. We can have a not-yet-enforced expectations that contributors run clang-format, and start enforcing it in 1-2 weeks once everybody has had a bit of experience with it.
  2. Let's not make changes to actual files in this patch.

So, basically, let's land the changes to .clang-format, give ourselves 2 weeks to get accustomed, and then make it hard-fail in the CI. For the next 2 weeks, there will be a not-CI-enforced-yet expectation that people clang-format their changes. That will let us adjust if needed.

This revision is now accepted and ready to land.Mon, Jun 13, 2:54 PM
EricWF requested changes to this revision.Mon, Jun 13, 3:57 PM
EricWF added a subscriber: EricWF.

This is not perfect, but it looks reasonable to me. However, some comments;

  1. Let's keep it soft-fail for a bit until we've gained some hands-on experience with formatting our code. We can have a not-yet-enforced expectations that contributors run clang-format, and start enforcing it in 1-2 weeks once everybody has had a bit of experience with it.
  2. Let's not make changes to actual files in this patch.

So, basically, let's land the changes to .clang-format, give ourselves 2 weeks to get accustomed, and then make it hard-fail in the CI. For the next 2 weeks, there will be a not-CI-enforced-yet expectation that people clang-format their changes. That will let us adjust if needed.

Absolutely requiring clang-format on _all_ code is going too far.

Clang-format should be available for contributors to use on their changes, and if they do, reviewers should not ask them to change the formatting.
But if a human wants to make code more human readable using whitespace, I say all the power too them; they can often out-format the tool.

Like all things, the value of a formatting tool, is not found in the extremes of never allowing or always mandating its use.
Instead, it's found in between; only when it's the right tool for the job.

Let's admit that having a clang-format spec is needed so people _can_ use the tool, but in doesn't and shouldn't require that they _must_ use the tool.

Nor should we reformat the entire world at once.
Let's go slowly, see how it works, and if it does work well, the codebase will get organically formatted over time.

In my opinion the VCS blame is far more important than having tool formatted code (especially if we might change the style and reformat more than once).

This revision now requires changes to proceed.Mon, Jun 13, 3:57 PM

To clarify, I have no problem with the suggested style.

I'm only blocking over the buildkite changes to make non-formatted code an error.

Yeah, I think that's a reasonable path forward. Previously we had agreed to make it a hard error, but I'm OK with taking it slow. In all cases, I think we're all on the same page that this very patch should be the .clang-format changes and only that.

Personally I don't see much of a reason to stay close to the LLVM style. We already deviate quite a bit to make the code readable. The libc++ code base is quite different from most of the other LLVM projects. Is there any specific reason you want to stay close to the LLVM style @Mordante?

Consistency in the entire project. I agree that libc++ requires some tweaking but the change I commented on seems not really needed for libc++. I personally prefer the amount of deviation of the LLVM style as small as possible.

Absolutely requiring clang-format on _all_ code is going too far.

Clang-format should be available for contributors to use on their changes, and if they do, reviewers should not ask them to change the formatting.
But if a human wants to make code more human readable using whitespace, I say all the power too them; they can often out-format the tool.

I agree it's easy to out-format the code manually, but in my experience that out-formatted code quickly bitrots to ugly formatted code. I don't object to manually formatted code as long as we accept clang-formatted code in followup patches and not require all changes to have manual formatting too.

Nor should we reformat the entire world at once.
Let's go slowly, see how it works, and if it does work well, the codebase will get organically formatted over time.

Agreed.

In my opinion the VCS blame is far more important than having tool formatted code (especially if we might change the style and reformat more than once).

Note LLVM already has a top-level .git-blame-ignore-revs file. So if we decide to do large scale formatting we can still keep git blame working nicely.

philnik updated this revision to Diff 436801.Tue, Jun 14, 8:30 AM

Only change the .clang-format

@ldionne @EricWF The .clang-format currently requires clang-format 15, so this can only be enforced after the release branch. So we have enough time to evaluate it.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jun 14, 11:12 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.