[PATCH] D113319: [clang-format] Improve require and concept handling

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks marked 22 inline comments as done.
HazardyKnusperkeks added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3516-3518
+    The clause tries to stick to the template declaration in case of class
+    templates or between template and function declarations. In case of
+    after the function declaration it tries to stick to this.
----------------
curdeius wrote:
> Just a suggestion, I'm not a writer. I'd just like to see something clear and comprehensible.
I'm happy to reword the stuff. :)


================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:1499-1500
+  if (State.NextToken->ClosesRequiresClause && Style.IndentRequiresClause) {
+    // Remove the indentation of the requires clauses (which is not in Indent,
+    // but in LastSpace).
+    State.Stack.back().LastSpace -= Style.IndentWidth;
----------------
curdeius wrote:
> And why it is not in `Indent`?
This one I tackled a month ago...
If I remember correctly this is because the indentation is not from an increased ``Level`` of the line, but because the position comes out of ``getNewLineColumn``.
The Level would only work if I would generate different UnwrappedLines depending on the clause style, which currently I don't.
The ``Indent`` is correctly reduced, but ``LastSpace`` stayed.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3900-3908
+  if (Right.is(TT_RequiresClause)) {
+    switch (Style.RequiresClausePosition) {
+    case FormatStyle::RCPS_OwnLine:
+    case FormatStyle::RCPS_ToFollowing:
+      return true;
+    default:
+      break;
----------------
curdeius wrote:
> The body of seems to be the exact same code as below in lines 3920-3926.
> Maybe you can refactor to something like this?
> Name of the lambda in my suggestion is maybe incorrect though.
Here is not about indentation, it is about breaking the line. And the difference is ``WithFollowing`` vs. ``WithPreceding``.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:731
+                                bool CanContainBracedList,
+                                TokenType NextLBracesType) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
----------------
curdeius wrote:
> Why plural?
Because it applies to all braces found within this block (not nested).


================
Comment at: clang/unittests/Format/FormatTest.cpp:3819-3822
+  verifyFormat("template <int I>\n"
+               "constexpr void foo\n"
+               "  requires(I == 42)\n"
+               "{}\n"
----------------
curdeius wrote:
> Do you test what was here previously with `RequiresClausePosition: SingleLine` (or whichever relevant option)?
I don't understand the question.

Before we had no real handling of requires, which did something like a combination of ``SingleLine`` and ``WithFollowing``.

I could have left the string in the test unchanged, but would have to set the style to ``SingleLine``.

I thought it would be acceptable to change the test, since we basically only now defined a default for LLVMStyle.


================
Comment at: clang/unittests/Format/FormatTest.cpp:23249-23252
+  verifyFormat("template <typename T>\n"
+               "concept C = [] -> bool { return true; }() && requires(T t) { "
+               "t.bar(); } &&\n"
+               "                      sizeof(T) <= 8;");
----------------
curdeius wrote:
> How about adding a test case with the exact same concept body but surrounded with parens (e.g. using `decltype(...)`)?
> The one below looks *almost* the same, but uses `std::true_type` and `::value` and so it's harder to transpose to what this test should look like.
Where should the parens go?
The misformatting is that `sizeof` is indented below `bool`, not below the `l_square`.


================
Comment at: clang/unittests/Format/FormatTest.cpp:23417
+      "              requires Bar<T> && Foo<T>;\n"
+      "              requires((trait<T> && Baz) || (T2<T> && Foo<T>));\n"
+      "            };",
----------------
curdeius wrote:
> curdeius wrote:
> > Hmm, no space after `requires` here?
> > It looks strange.
> > My preference would be not to put a space before a parameter list (as in `requires(T t)`), but put a space before of a constraint expression.
> > It seems to be the style used in https://en.cppreference.com/w/cpp/language/constraints.
> > Unless there are options that modify this behaviour?
> Oh, I've just seen a comment below on basically the same topic.
Yeah, the change for that is D113369 which I will update shortly.

We can gladly discuss the defaults there.


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

https://reviews.llvm.org/D113319