[PATCH] D113319: [clang-format] Improve require and concept handling
Arthur O'Dwyer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Dec 4 07:14:08 PST 2021
Quuxplusone 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>
----------------
HazardyKnusperkeks wrote:
> 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.
> it was really easy to add
Even easier not to add. ;)
================
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;
----------------
HazardyKnusperkeks wrote:
> 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?
Well, I see that there is a machine-readable distinction between "`requires` after `template`" versus "`requires` after function parameter list," i.e. between
```
template<class T> requires X
void f(T);
template<class T>
void f(T) requires X;
```
The latter position is not (currently) valid for non-functions (e.g. `template<class T> int v requires X;` is a syntax error).
However, as usual, I don't think that the user should //want// to format these differently.
If you're using clang-format to format your code, then you should definitely want function-parameter-list-requires-clauses on their own line for sure, to avoid confusing strings of tokens such as
```
void f() const noexcept requires foobar<T>; // bad
void f() const noexcept // better
requires foobar<T>;
```
If you do that, then (in my proposed world) you would also automatically get
```
template<class T>
requires foobar<T> // automatically
void f();
template<class T> requires foobar<T> // can't get this
void f();
template<foobar T> // could also get this, via manual intervention
void f();
```
And personally I see nothing wrong with that state of affairs.
================
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;");
----------------
HazardyKnusperkeks wrote:
> 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.
I guess the higher-level question (which I didn't think of until seeing many other tests) is, redesign this test case to be more targeted and thus shorter. E.g. could you `s/std::trait<T>::value/Bar/` or would that destroy the point of the test? (I'm not sure because I'm not sure what the point of the test is.)
================
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;");
----------------
HazardyKnusperkeks wrote:
> 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.
OK, if clang-format by default likes to align subexpressions vertically like that, then I agree with your formatting. Either way, though, what it's currently producing is incorrect. :)
================
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;",
----------------
HazardyKnusperkeks wrote:
> 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.
Thanks, that resolves this comment! (I left some review comments over there: I agree with that one's two-option approach even though I suggest better //names// for the two options.)
================
Comment at: clang/unittests/Format/FormatTest.cpp:23268-23270
+ return;
+ FormatStyle Style = getLLVMStyle();
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
----------------
HazardyKnusperkeks wrote:
> 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.)
> other than changing what is released feels a bit wrong
IIUC, //nothing// is released in this area right now. clang-format doesn't indent `requires`-clauses, sure, but that's because it doesn't understand the keyword. Once clang-format understands C++20, it makes perfect sense to me that we'd teach it to format C++20 in LLVM style, as opposed to formatting C++20 in "a style designed merely to imitate the previous buggy behavior." That'd be like... neoclassical buildings and sculptures being painted white to imitate ancient buildings and sculptures whose paint had worn off. It's not that the ancients //liked// white (it's not that LLVM 13 //liked// unindented requires-clauses); it's that their surviving artifacts (clang13-formatted codebases) reflect only a "decayed" version of their actual preferences, because of limitations in their technology.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113319/new/
https://reviews.llvm.org/D113319
More information about the cfe-commits
mailing list