[libc++] Update clang-format style.

[Mordante]

Summary

After evaluating the new style I noticed inner namespaces are now
indented. I am not fond of that style and I've seen some other review
comment in this regard so I propose we remove this option and use the
LLVM default not to indent it.

[philnik]

For the record, I asked @var-const on Discord and clarified a bit:

@philnik
What exactly is you question on D124789? Do you want to know what the setting does, or are you asking why I set it?

@var-const
Sorry for not being clear -- asking why this setting is being set. IIUC, it diverges from LLVM (and from my preferred style 🙂 ), so I'm curious why you decided to do this change

@philnik
Well, the short answer is: It's my preferred style (I mostly copied my own clang-format and tweaked it to 120 lines a good as possible and to requests from other people).
The answer to why it's my preferred style is that it shows interesting/unusual code. Having a single namespace is normal (std::__1::{ranges, filesystem, ...} in the case of libc++). Anything beyond that is something you might want to look at (i.e. why isn't it collapsed to a single namespace std::__1::ranges::whatever/what namespace am I actually in). This also discourages complex namespace structures instead of collapsing it to a single namespace declaration and re-opening the namespace a second time if you need to. This of course only works for C++17-and-newer code, but I don't think that's a problem in libc++. i.e.if you need ranges::__copy::__impl or whatever you should write namespace ranges { /*stuff*/ } namespace ranges::__copy { /*stuff*/ } namespace ranges::__copy::__impl { /*stuff*/ } instead of namespace ranges { /*stuff*/ namespace __copy { /*stuff*/ namespace __impl { /*stuff*/ } } }.

@var-const
Thanks for explaining (and sorry I missed it in the patch). My preference is to avoid extra indentation. I feel that any extra indentation is mostly useful within a short scope, where a difference in indentation is a great visual aid to separate control structures from the "main" flow of the function, for example. However, typically contents of a namespace would be longer than a single screen, meaning that, in most cases, everything the reader sees will have an extra level of indentation. This way, indentation no longer helps distinguish between anything and effectively just reduces the column limit (which we generally want to extend). I could agree that it makes sense to indent short namespaces where contents fit into a screen, but that spawns the question of where to draw the line (and is it worthwhile to have different rules for these cases).

I could also agree about visually indicating "interesting" namespaces, but I think that having an unnamed namespace in a source file or, similarly, some kind of an impl namespace in a header file is pretty common and not worth calling out. While closing and reopening the current namespace would work, it feels like a workaround.

I think we can agree that it doesn't work very well in libc++ as-is. However, I think it would be a good idea to have non-standard namespaces indicated in some way. I don't know how that should look, so I don't think it's a blocker.

[philnik]

(Apparently Ctrl + Enter is sending a message, who knew.)
My main concern with this is that I think there should be more changes made to the clang-format. I think we should put all the changes we want to make right now into a single PR. I don't know if you have had any more problems with the clang-format until know, but I know I had. I propose to change PenaltyIndentedWhitespace to two. I've tried it in a couple of PRs and that fixes the problem that clang-format produces weird code like

template <
    class _Type,
    class _Alloc,
    class... _Args,
    class = __enable_if_t<!is_same_v<
        decltype(std::__uses_allocator_construction_args_impl<_Type>(
            std::declval<const _Alloc&>(), std::declval<_Args>()...)),
        void>>>

and changes the code to

template <class _Type,
          class _Alloc,
          class... _Args,
          class = __enable_if_t<!is_same_v<decltype(std::__uses_allocator_construction_args_impl<_Type>(
                                               std::declval<const _Alloc&>(), std::declval<_Args>()...)),
                                           void>>>

which is much better IMO. Putting it lower than two produces declarations where the parameters are almost all the way to the left, which I think isn't what we want either.

[Mordante]

This change was the only major issue I had with the current style. There are some other things that I don't really like but nothing major. I feel AlignConsecutiveAssignments is a bit of a hit or miss, depending on the code it looks great or not really better than without this option. But it doesn't bother me.

I've no strong opinion on PenaltyIndentedWhitespace but the new value looks slightly better.

I'll wait for comments of other reviewers and then apply other requested changes.

[philnik]

I definitely agree that AlignConsecutiveAssignments is very hit-or-miss. I originally put that in to align consecutive using declarations. Maybe we can request an option for that. IMO it increases the readability a lot there.

[ldionne]

+1 on PenaltyIndentedWhitespace.

Regarding indenting inner namespaces, my opinion (which has been expressed by @philnik) is that indenting small less-than-one-screen namespaces is useful, but longer ones not so much. Would it be possible to handle that in clang-format? Maybe the option could be something like NamespaceIndentationThreshold: 50 (namespaces 50 lines or less are indented, others are not).

Also, it would be useful to format one file as part of this review just to show how this changes -- it can be omitted upon committing.

[Mordante]

DO NOT LAND
Just an example how things look with the current formatting and
PenaltyIndentedWhitespace: 2

However the main point is to look at NamespaceIndentation: Inner

[Mordante]

One of the other gripes I have about NamespaceIndentation that it doesn't work as advertised, namespaces like chrono should be indented due to being an inner namespace. So the indention for our code bases is inconsistent. (That might be as wanted, but it would break if at one point clang-format becomes smarter.)

[ldionne]

I'm happy either way, so the patch as-is (minus the changes to parser_std_format_spec.h) LGTM.

Like I said, I have a slight preference for indenting small-ish detail namespaces when it increases legibility and I might keep doing that under // clang-format off, but I'm happy to go with whatever gets us consensus here.

libcxx/.clang-format

 BreakInheritanceList: BeforeColon
 EmptyLineAfterAccessModifier: Never
 EmptyLineBeforeAccessModifier: Always
 IndentWrappedFunctionNames: false
 IndentRequires: true
 InsertTrailingCommas: Wrapped
 KeepEmptyLinesAtTheStartOfBlocks: false
 MaxEmptyLinesToKeep: 1
-NamespaceIndentation: Inner
 PackConstructorInitializers: NextLine
 
-PenaltyIndentedWhitespace: 61
+PenaltyIndentedWhitespace: 2
 
 Language: Cpp
 Standard: c++20
 SpacesInAngles: Leave
 
 AlwaysBreakTemplateDeclarations: true
 PointerAlignment: Left