[libcxx-commits] [PATCH] D99691: [libcxx] adjusts formatting rules

Thu Apr 1 14:23:36 PDT 2021

ldionne added a comment.

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.

================
Comment at: libcxx/.clang-format:5
 Language: Cpp
-Standard: Cpp03
+Standard: Cpp11

----------------
Quuxplusone wrote:
> curdeius wrote:
> > Mordante wrote:
> > > curdeius wrote:
> > > > Mordante wrote:
> > > > > Quuxplusone wrote:
> > > > > > 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`.
> > > > >  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.
> > > > 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.
> > > > 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?
> > > 
> > > 
> > 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.
> > 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'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.

================
Comment at: libcxx/.clang-format:16
+BreakBeforeConceptDeclarations: true
+IndentRequires: false
 ---
----------------
cjdb wrote:
> Quuxplusone wrote:
> > 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) {
> > ```
> 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99691/new/

https://reviews.llvm.org/D99691