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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

I can just say: ship it!

Thanks for all the work!



================
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;
----------------
HazardyKnusperkeks wrote:
> 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.
Ok. Roger.


================
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;
----------------
HazardyKnusperkeks wrote:
> 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``.
That's what I meant saying that the name might be  incorrect.


================
Comment at: clang/unittests/Format/FormatTest.cpp:3819-3822
+  verifyFormat("template <int I>\n"
+               "constexpr void foo\n"
+               "  requires(I == 42)\n"
+               "{}\n"
----------------
HazardyKnusperkeks wrote:
> 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.
Fair enough.


================
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;");
----------------
HazardyKnusperkeks wrote:
> 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`.
Oh, right.


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

https://reviews.llvm.org/D113319