Quuxplusone added inline comments.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3424
+ templates or between template and function delcarations. In case of
+ after the function delcaration it tries to stick to this.
+
----------------
`s/delca/decla/g`
`s/Preceeding/Preceding/g`
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3464-3483
+ * ``RCPS_TwoLines`` (in configuration: ``TwoLines``)
+ The clause always gets its own line, and the content of the clause go
+ into the next line with some indentation.
+
+ .. code-block:: c++
+
+ template<typename T>
----------------
This option strikes me as actively harmful; I think you should remove it from
the PR completely. Nobody does this today and nobody should do it in the future
either.
================
Comment at: clang/include/clang/Format/Format.h:2504-2505
- /// Indent the requires clause in a template
+ /// Indent the requires clause in a template, this only applies if the
+ /// ``requires`` is at the beginning of a line.
/// \code
----------------
Comma-splice. But I don't think you need to change this comment at all. It's
obvious that "indent" applies only to constructs at the left side of the
(logical) source line.
================
Comment at: clang/include/clang/Format/Format.h:3130-3136
+ /// \brief The position of the ``requires`` clause for class templates.
+ /// \version 14
+ RequiresClausePositionStyle RequiresClausePositionForClasses;
+
+ /// \brief The position of the ``requires`` clause for function templates.
+ /// \version 14
+ RequiresClausePositionStyle RequiresClausePositionForFunctions;
----------------
What about for variable templates? What about for alias templates? What about
for deduction guides?
It makes sense to me to have //one// option for this. It doesn't make sense to
have 2, 3, or 4. Having 5 feels insane. So, I think you should just have one
option that applies consistently to all `requires`-clauses everywhere in the
code.
================
Comment at: clang/lib/Format/Format.cpp:1213
+ // This is open for discussions! When will LLVM adapt C++20?
+ LLVMStyle.RequiresClausePositionForClasses = FormatStyle::RCPS_OwnLine;
+ LLVMStyle.RequiresClausePositionForFunctions = FormatStyle::RCPS_OwnLine;
----------------
HazardyKnusperkeks wrote:
> What are your opinions on that?
+1 `RequiresClausePosition=OwnLine`, but also `IndentRequires=true`.
================
Comment at: clang/unittests/Format/FormatTest.cpp:22288-22291
+ verifyFormat("template <typename T>\n"
+ "concept C = ((false || foo()) && C2<T>) || "
+ "(std::trait<T>::value && Baz) ||\n "
+ " sizeof(T) >= 6;");
----------------
Nit: I was initially very confused by the formatting here, until I realized
that lines 22289 and 22290 are two lines //physically// but represent one
single line in the //test case//. Strongly recommend eliminating the gratuitous
line break.
================
Comment at: clang/unittests/Format/FormatTest.cpp:22374-22378
+ "template <typename T>\n"
+ "concept C = decltype([]() { return std::true_type{}; }())::value &&\n"
+ " requires(T t) {\n"
+ " t.bar();\n"
+ "} && sizeof(T) <= 8;");
----------------
This looks wrong. Current:
```
template <typename T>\n"
concept C = decltype([]() { return std::true_type{}; }())::value &&
requires(T t) {
t.bar();
} && sizeof(T) <= 8;
```
but should be:
```
template <typename T>\n"
concept C = decltype([]() { return std::true_type{}; }())::value &&
requires(T t) {
t.bar();
} && sizeof(T) <= 8;
```
(assuming that all the relevant indent-widths are set to `2`).
================
Comment at: clang/unittests/Format/FormatTest.cpp:22597
+ verifyFormat("template <std::semiregular F, std::semiregular... Args>\n"
+ "requires(std::invocable<F, std::invoke_result_t<Args>...>)\n"
+ "struct constant;",
----------------
Currently:
```
template <std::semiregular F, std::semiregular... Args>
requires(std::invocable<F, std::invoke_result_t<Args>...>)
struct constant;
```
Should be:
```
template <std::semiregular F, std::semiregular... Args>
requires (std::invocable<F, std::invoke_result_t<Args>...>)
struct constant;
```
(notice the extra single space). Add a TODO comment and ship it anyway?
================
Comment at: clang/unittests/Format/FormatTest.cpp:22643
verifyFormat("template <typename T>\n"
- "concept Context = Traits<typename T::traits_type> or\n"
- " Interface<typename T::interface_type> or\n"
- " Request<typename T::request_type> or\n"
- " Response<typename T::response_type> or\n"
- " ContextExtension<typename T::extension_type> or\n"
- " ::std::is_copy_constructable<T> or "
- "::std::is_move_constructable<T> or\n"
- " requires (T c) {\n"
- " { c.response; } -> Response;\n"
- "} or requires (T c) {\n"
- " { c.request; } -> Request;\n"
- "}\n",
+ "requires requires(T t) {\n"
+ " typename T::Bar;\n"
----------------
Data point: libc++ has a mix of `requires (T t) { }` and `requires(T t) { }`.
Am I correct that clang-format is not yet smart enough to have a meaningful
opinion about this particular whitespace?
I would bet that two years ago I was doing `requires(T t) { }`, but these days
`requires (T t) { }` is much more natural to me.
================
Comment at: clang/unittests/Format/FormatTest.cpp:22715-22717
+ " typename T::Bar;\n"
+ " { t.bar() } -> std::same_as<bool>;\n"
+ "}\n"
----------------
HazardyKnusperkeks wrote:
> Should these lines be indented because requires is indented, or not?
> Should these lines be indented because requires is indented, or not?
================
Comment at: clang/unittests/Format/FormatTest.cpp:22715-22717
+ " typename T::Bar;\n"
+ " { t.bar() } -> std::same_as<bool>;\n"
+ "}\n"
----------------
Quuxplusone wrote:
> HazardyKnusperkeks wrote:
> > Should these lines be indented because requires is indented, or not?
> > Should these lines be indented because requires is indented, or not?
>
>
Yes.
```
template<typename T>
requires requires (T t) {
typename T::Bar;
{ t.bar(); } -> std::same_as<bool>;
}
class Foo {
};
```
(And in general it would be nice to simplify all of these test cases: lines
22707–22709, 22719–22721, etc. definitely aren't needed, and I even question
whether line 22716 is germane to the point of this test case. Ideally, each
test case will regression-test a single specific functionality or codepath,
with no unnecessary digressions.)
================
Comment at: clang/unittests/Format/FormatTest.cpp:22812-22814
+ verifyFormat("template <std::semiregular F, std::semiregular... Args>\n"
+ " requires true struct constant;",
+ Style);
----------------
This result of `RequiresClausePosition=SingleLine, IndentRequires=true` feels
bizarre to me. I'm 99% sure that `RequiresClausePosition=SingleLine` should
implicitly disable `IndentRequires`, because `IndentRequires` applies only to
requires-clauses at the beginning of a logical source line, and with
`SingleLine` a requires-clause is //never// at the beginning of a logical
source line.
The fact that this `requires` keyword accidentally fell at the beginning of a
//physical// source line should be completely irrelevant to the algorithm.
We should test a progression that looks like
```
template<VeryLongName F> requires
true struct S;
template<VeryLongName123456789 F>
requires true struct S;
template<VeryLongName1234567890 F
> requires true struct S;
```
(But also, `SingleLine` is a bad style and it would be cool to just not support
it at all. Nobody should use it.)
================
Comment at: clang/unittests/Format/FormatTest.cpp:23268-23270
+ return;
+ FormatStyle Style = getLLVMStyle();
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
----------------
(1) That `return` on line 23268 is sneaky. 😛
(2) LLVM style should certainly set `IndentRequires=true`, so this test
shouldn't pass without some modification to line 23273.
================
Comment at: clang/unittests/Format/FormatTest.cpp:23441
+ "void func(T) {}");
+ verifyFormat("template <tyename T>\n"
+ "requires requires(T &&t) {\n"
----------------
`s/tyename/typename/g`
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
