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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
