HazardyKnusperkeks marked 4 inline comments as done.
HazardyKnusperkeks added inline comments.
================
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>
----------------
Quuxplusone wrote:
> 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.
I will never use it, so I have no strong opinion in that, because it was really
easy to add. I will let others decide if we want to offer it.
================
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;
----------------
Quuxplusone wrote:
> 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.
That makes sense, in my defense I thought about it and came to the conclusion
there would be no need for requires on variable templates, and the others I did
not think of.
Why I have chosen to options is maybe someone wants
```template <typename T>
requires ...
class ...```
But also
```template <typename T>
void foo(T) requires ... {
```
What's your opinion on that?
================
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;");
----------------
Quuxplusone wrote:
> 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.
I'm not really happy wit the two solutions which come to my mind:
Using a Style with a lower column limit, or using // clang format off.
================
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;");
----------------
Quuxplusone wrote:
> 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`).
For me personally it should look
```template <typename T>\n"
concept C = decltype([]() { return std::true_type{}; }())::value &&
requires(T t) {
t.bar();
} && sizeof(T) <= 8;```
I have not taken any action to manipulate the position, this is what dropped
out of clang-format naturally. And I think the requires should start below the
decltype the same as all other conditions would. If the body should be indented
is open for discussion.
================
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;",
----------------
Quuxplusone wrote:
> 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?
Take a look at D113369 which will most likely land simultaneously with this
change, but currently needs to be updated.
================
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"
----------------
Quuxplusone wrote:
> 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.
See above.
================
Comment at: clang/unittests/Format/FormatTest.cpp:23268-23270
+ return;
+ FormatStyle Style = getLLVMStyle();
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
----------------
Quuxplusone wrote:
> (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.
This test case will be gone, after I'm done. I just keep it to look for some
variants I might have missed in writing new tests.
About `IndentRequires` I have strong opinion, other than changing what is
released feels a bit wrong. (Since as far as I can see LLVM Style does not
mention requires at all.)
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
