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

[Björn Schäpers via Phabricator via cfe-commits]

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