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