This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adjusts formatting rules
ClosedPublic

Authored by cjdb on Mar 31 2021, 3:51 PM.

Details

Reviewers
ldionne
zoecarver
Group Reviewers
Restricted Project
Commits
rG2e3a78b8ca10: [libcxx][NFC] adjusts formatting rules
Summary

This will reduce the amount of noisy feedback during reviews.

Diff Detail

Event Timeline

cjdb requested review of this revision.Mar 31 2021, 3:51 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 3:51 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
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 added inline comments.Mar 31 2021, 9:43 PM
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.

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 added inline comments.
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.

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

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.

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.

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.

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 added inline comments.Apr 1 2021, 4:15 AM
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 added inline comments.Apr 1 2021, 9:08 AM
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.)

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 updated this revision to Diff 334828.Apr 1 2021, 2:34 PM

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

cjdb marked 9 inline comments as done.Apr 1 2021, 3:07 PM

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.

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";.

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.

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 accepted this revision as: zoecarver.Apr 8 2021, 11:35 AM
zoecarver added a subscriber: 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.

ldionne accepted this revision.Apr 14 2021, 1:57 PM
This revision is now accepted and ready to land.Apr 14 2021, 1:57 PM
cjdb marked an inline comment as done.Apr 14 2021, 7:46 PM

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.

This revision was automatically updated to reflect the committed changes.