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.
Details
Details
- Reviewers
ldionne Mordante var-const EricWF - Group Reviewers
Restricted Project - Commits
- rGe7c72d69ac0d: [libc++] Find a clang-format everybody is happy with
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
First two questions that come to my mind (we can move to Discord if you prefer):
- 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).
Comment Actions
- 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.
Comment Actions
- 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.
| 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. |
Comment Actions
- 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.
Comment Actions
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. |
Comment Actions
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. |
Comment Actions
This is not perfect, but it looks reasonable to me. However, some comments;
- 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.
- 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.
Comment Actions
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).
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
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.
| libcxx/.clang-format | ||
|---|---|---|
| 68 | What is the purpose of this setting? IIRC, LLVM never indents namespaces, which I think reduces unnecessary indentation and makes things easier to read. | |
| libcxx/.clang-format | ||
|---|---|---|
| 68 | +1 I actually found this independently. I also feel it works quite misleading in our codebase since _LIBCPP_BEGIN_NAMESPACE_STD doesn't seem to be counted as a namespace. | |
This seems to deviate from LLVM style, why do we want to deviate here?