[libcxx] adjusts formatting rules

[cjdb]

Summary

This will reduce the amount of noisy feedback during reviews.

[• Quuxplusone]

libcxx/.clang-format

5

Actually, I'd turn this all the way up to Cpp20 or however high it goes. The only interesting thing controlled by this option AFAIK is whether clang-format will do

foo<bar<baz> > x;  // we must do this anywhere C++03-portability is required

or

foo<bar<baz>> x;  // we prefer this wherever possible

Either way, blindly applying clang-format to actual libc++ code is going to screw it up one way or the other. But given that you're the only person relying on clang-format to format your patches AFAIK, and you're working exclusively on C++20 code (never on C++03 code), I think it's totally reasonable to check in a style file that specifies Cpp20.

16

Please set "IndentRequires" to true. Or at least, regardless of what tools you run locally to format your code, please ensure that what gets checked in looks like

template <class _Tp>
  requires foo<_Tp>
inline constexpr bar(_Tp) {

and not

template <class _Tp>
requires foo<_Tp>
inline constexpr bar(_Tp) {

[cjdb]

libcxx/.clang-format

16

The current libc++ style is as if IndentRequires is false and I'd prefer to keep it that way; unless @ldionne asks it to be changed.

[Mordante]

I really like you're working on this!

I'm only concerned the new formatting rules will break valid C++98 code.

libcxx/.clang-format

5

I also rely on clang-format for my patches and I'm not fond that this change allows to break C++98 code. So I prefer std::vector<std::pair<int, int> > over std::vector<std::pair<int, int>> and my C++03 unit tests break. Ideally I would like to have this option in clang format

// clang-format Cpp03
std::vector<std::pair<int, int> > v3;
// clang-format Cpp11
std::vector<std::pair<int, int>> v11;

Then we can use a sane default and override it where applicable. I'll reach out to the clang-format developers to see how feasible this would be.

[curdeius]

libcxx/.clang-format

5

Same for me. Changing to Standard: Cpp11 will possibly break code with no huge benefit IMO.

Now, taking my clang-format dev's hat...
Concerning @Mordante's suggestion, that may be doable (preferably in a more general form e.g. // clang-format Option: Value or even // clang-format {Style...}), but not sure if worth doing, as it will possibly make the formatting inconsistent between different parts of the code if used injudiciously.
And, the whole purpose of using clang-format is consistency.

[curdeius]

Or, adding fuel to the fire, could we possibly deprecate C++03 mode in libc++?

[mstorsjo]

In D99691#2663091, @curdeius wrote:

Or, adding fuel to the fire, could we possibly deprecate C++03 mode in libc++?

Sorry, -1 from me on that one at least - I'm pretty sure that out of the software packages that still are regularly compiled with new toolchains these days, a not-insignificant number of them are still built in C++98/03 mode.

[curdeius]

In D99691#2663099, @mstorsjo wrote:

In D99691#2663091, @curdeius wrote:

Or, adding fuel to the fire, could we possibly deprecate C++03 mode in libc++?

Sorry, -1 from me on that one at least - I'm pretty sure that out of the software packages that still are regularly compiled with new toolchains these days, a not-insignificant number of them are still built in C++98/03 mode.

But do such projects use the latest version of the standard library?
Anyway, it was half a joke to propose the deprecation and the discussion about it is out of scope of this patch.

[mstorsjo]

In D99691#2663291, @curdeius wrote:

In D99691#2663099, @mstorsjo wrote:

In D99691#2663091, @curdeius wrote:

Or, adding fuel to the fire, could we possibly deprecate C++03 mode in libc++?

Sorry, -1 from me on that one at least - I'm pretty sure that out of the software packages that still are regularly compiled with new toolchains these days, a not-insignificant number of them are still built in C++98/03 mode.

But do such projects use the latest version of the standard library?

I would think so. Consider e.g. a distribution - a linux distribution could be using libc++ as default (I'm not off-hand aware of any doing that). Or MSYS2 are considering a separate package namespace for all of their packages, built with clang/lld/libc++. They have ~1850 packages from various sources, that would be rebuilt with the latest and greatest toolchain. And my own toolchain packaging, llvm-mingw, which I test (with the latest nightly of everything from llvm/libc++) against building a bunch of projects, including VLC (which bundles a bit over 100 third party libraries). I'd expect Chromium to be doing pretty much the same too.

So rebuilding a large array of existing projects regularly with a new compiler isn't anything strange in itself. For projects that are incompatible with newer versions, I believe they might have started (either upstream or via downstream patches) to set -std=c++98) after compilers started defaulting to newer standards.

Anyway, it was half a joke to propose the deprecation and the discussion about it is out of scope of this patch.

Yeah, clear out of scope here, just replying to give some context.

[Mordante]

In D99691#2663091, @curdeius wrote:

Or, adding fuel to the fire, could we possibly deprecate C++03 mode in libc++?

Also a -1 for me. We have quite an amount of unit tests using C++03. I really dislike clang-format to automatically breaking these tests when formatting.

libcxx/.clang-format

5

Now, taking my clang-format dev's hat...
Concerning @Mordante's suggestion, that may be doable (preferably in a more general form e.g. // clang-format Option: Value or even // clang-format {Style...}), but not sure if worth doing, as it will possibly make the formatting inconsistent between different parts of the code if used injudiciously.
And, the whole purpose of using clang-format is consistency.

I also love consistency but if a file has C++20 and C++03 code I wouldn't mind to have them format differently based on the language features. I think our mixing of C++ version is uncommon, mainly standard libraries and boost so I wouldn't mind if the // clang-format CppXX may only be used once in a file and affects the entire file. (Since I'm no clang-format dev I leave the best naming to you.)

Do you have better suggestions how mixing the C++ version on a per file basis can be achieved easier in clang-format?

[curdeius]

libcxx/.clang-format

5

No, I don't have for what clang-format offers now.
It's a pity though that there's no option SpacesInAngles: Keep/Leave (because we are speaking of this aspect that Standard option changes, at least partially as SpacesInAngles: true adds space after the opening < as well). This might be the best way to go on a longer term (~release 13 at least).
Personally, I would keep Standard option as is for the moment and wait for SpacesInAngles: Keep/Leave in clang-format.

[• Quuxplusone]

libcxx/.clang-format

5

Personally, I would keep Standard option as is for the moment and wait for SpacesInAngles: Keep/Leave in clang-format.

Personally, I would keep this whole .clang-format file as-is, or even git rm it entirely, and just remember not to use clang-format to format libc++ submissions because clang-format can't format libc++ submissions correctly.

I also wouldn't like to see people sprinkling //clang-format {on,off,Cpp03,Cpp11,whatever} comments throughout their submissions — that kind of comment is not useful to the reader, and again is useful to the writer only if he's using clang-format blindly. (Which they're probably doing in the name of "consistency," which goal is defeated by sprinkling these comments everywhere.)

[ldionne]

I think this is great, with nitpicks. I've actually been thinking that we should make the clang-format CI job a hard-failure, as that would greatly reduce the room for formatting related comments, which are often (although not always) a distraction.

Even though we all have pet peeves and opinions w.r.t. formatting, I think we'll all be more productive and will have a easier time collaborating if we just agree on one style guide that can be applied mechanically. It's hard for everyone to make the transition, myself included (I love my style!), but I think it's the pragmatic way forward.

libcxx/.clang-format

5

I'll disregard the rest of this comment thread and simply say that we can unfortunately not break C++03. In the spirit of being able to clang-format arbitrary pieces of code in libc++ without having to think too much, I would not bump this to Cpp11.

16

Frankly, I have no real opinion about this. I think we should go with whatever we think is most readable without concern for how existing code has been written. I'd leave it to @cjdb to decide, since you've been writing pretty much all the concepts we have so far. Just pick what you think is most readable.

[cjdb]

reverts Cpp11 to Cpp03 due to points about this breaking lots of supported C++03 code.

[cjdb]

In D99691#2665215, @ldionne wrote:

I think this is great, with nitpicks. I've actually been thinking that we should make the clang-format CI job a hard-failure, as that would greatly reduce the room for formatting related comments, which are often (although not always) a distraction.

Is that something I can do in this commit?

Even though we all have pet peeves and opinions w.r.t. formatting, I think we'll all be more productive and will have a easier time collaborating if we just agree on one style guide that can be applied mechanically. It's hard for everyone to make the transition, myself included (I love my style!), but I think it's the pragmatic way forward.

Please. I find comments asking for > > to be turned into >>, and friends, exhausting.

libcxx/.clang-format

16

IndentRequires: false shall remain then.

[Mordante]

In D99691#2665215, @ldionne wrote:

I think this is great, with nitpicks. I've actually been thinking that we should make the clang-format CI job a hard-failure, as that would greatly reduce the room for formatting related comments, which are often (although not always) a distraction.

Even though we all have pet peeves and opinions w.r.t. formatting, I think we'll all be more productive and will have a easier time collaborating if we just agree on one style guide that can be applied mechanically. It's hard for everyone to make the transition, myself included (I love my style!), but I think it's the pragmatic way forward.

I'm really in favour if this. I've seen enough coding styles that I no longer care a lot about the exact style. The thing I care about is to have a tool to do the proper formatting for me.

Unfortunately there's a blocker to make clang-format a hard error. We need a solution to support multiple C++ version at least on a file basis. Using C++03 formatting will break valid C++20 code auto str = u8"abc"; will become auto str = u8 "abc";.

[curdeius]

In D99691#2667984, @Mordante wrote:

Unfortunately there's a blocker to make clang-format a hard error. We need a solution to support multiple C++ version at least on a file basis. Using C++03 formatting will break valid C++20 code auto str = u8"abc"; will become auto str = u8 "abc";.

That's a very good point.
I started working on SpacesInAngles: Leave option for clang-format. If that's accepted, we could use this option together with Standard: Latest (or c++20, btw, Cpp11 is deprecated). I need to check that there's nothing else apart angle brackets that may break pre-C++20 code.

[ldionne]

Is it possible to increase the length of lines as enforced by clang-format? Libc++ tends to have a lot of verbose identifiers and macros, and it really helps readability if that doesn't cause us to break lines all around the place.

[zoecarver]

I started working on SpacesInAngles: Leave option for clang-format. If that's accepted, we could use this option together with Standard: Latest (or c++20, btw, Cpp11 is deprecated). I need to check that there's nothing else apart angle brackets that may break pre-C++20 code.

Yay, that would be awesome.

Please. I find comments asking for > > to be turned into >>, and friends, exhausting.

Sorry :P

Is it possible to increase the length of lines as enforced by clang-format? Libc++ tends to have a lot of verbose identifiers and macros, and it really helps readability if that doesn't cause us to break lines all around the place.

We can use ColumnLimit. Or ColumnLimit: 0 to ignore it. I don't have a preference one way or another (in terms of leaving it, changing it, or removing it).

---

I think we should land this, or update the column limit and land this (though, I'd argue that should be a different patch). We can (and should) discuss the other things (such as deprecating C++03) in another place. This is a small, contained, easy, nfc patch. Let's just get it in.

[cjdb]

In D99691#2674136, @ldionne wrote:

Is it possible to increase the length of lines as enforced by clang-format? Libc++ tends to have a lot of verbose identifiers and macros, and it really helps readability if that doesn't cause us to break lines all around the place.

I've upped the length from 80 (default, per LLVM style) to 120. We can always tweak it if that feels too long or too short.

libcxx/.clang-format

 BasedOnStyle: LLVM
 
 ---
 Language: Cpp
 Standard: Cpp03
 
 AlwaysBreakTemplateDeclarations: true
 PointerAlignment: Left
 
 # Disable formatting options which may break tests.
 SortIncludes: false
 ReflowComments: false
+
+# libc++ has some long names so we need more than the 80 column limit imposed by LLVM style, for sensible formatting
+ColumnLimit: 120
+
+# These aren't covered by the LLVM Style Guide
+BreakBeforeConceptDeclarations: true
+IndentRequires: false
 ---