Page MenuHomePhabricator

[clang-format] Improve require and concept handling
AcceptedPublic

Authored by HazardyKnusperkeks on Nov 5 2021, 2:03 PM.

Details

Summary

Added two new options to control where requires clauses are positioned depending on whether they are for classes or functions.

Improved handling of concepts and requires expressions.

Fixes https://llvm.org/PR32165, https://llvm.org/PR32166, and https://llvm.org/PR52401.

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Nov 5 2021, 2:03 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2021, 2:03 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Or am I mistaken and we want to be able to control the line break?

Also I would break before struct, class, union, but I would have needed to change the test, and I know that we at least need to talk about in in front. :)

template<typename T>
require X<T>
class ...

vs.

template<typename T>
require X<T> class ...
clang/lib/Format/TokenAnnotator.cpp
1419

This one has cost me a lot of time, should we rethink this?

I feel anything you do here would be an improvement. its probably worth looking at recent issues logged around this issue at the same time

https://bugs.llvm.org/show_bug.cgi?id=52401
https://bugs.llvm.org/show_bug.cgi?id=32165

When I added this support its was basically to prevent it from mascaraing concepts, take my implementation with a grain of salt if you think it needs changing please feel free to propose it.

MyDeveloperDay accepted this revision.Nov 8 2021, 4:41 AM
This revision is now accepted and ready to land.Nov 8 2021, 4:41 AM
HazardyKnusperkeks planned changes to this revision.Nov 8 2021, 12:10 PM

I feel anything you do here would be an improvement. its probably worth looking at recent issues logged around this issue at the same time

https://bugs.llvm.org/show_bug.cgi?id=52401
https://bugs.llvm.org/show_bug.cgi?id=32165

When I added this support its was basically to prevent it from mascaraing concepts, take my implementation with a grain of salt if you think it needs changing please feel free to propose it.

I will take a look at the issues and incorporate them. I think I will add a few new options.

HazardyKnusperkeks retitled this revision from [clang-format] Improve require handling to [clang-format] Improve require and concept handling.
HazardyKnusperkeks edited the summary of this revision. (Show Details)

Update on my work in progress. The new test cases are all passed. The old ones are only kept to pick some lines from them.

I would like to hear some feedback while I finish the rest, mostly writing tests.

This revision is now accepted and ready to land.Dec 3 2021, 12:26 PM

Still WIP, not marking as changes planned, so that it pops up on your list and you can give me feedback. :)

clang/lib/Format/Format.cpp
1213 ↗(On Diff #391708)

What are your opinions on that?

clang/lib/Format/UnwrappedLineParser.cpp
368

Does anyone know for sure what the level is? I have an idea, but I'm not 100% sure yet.

2490

Here I have an requires for a requires clause which did not get a type. Same as requires for requires expressions right now.

Should I tag all of them, even if I do not use the type right now?

2536

This will be removed.

clang/unittests/Format/FormatTest.cpp
22280–22282

Should these lines be indented because requires is indented, or not?

Quuxplusone added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3424 ↗(On Diff #391708)

s/delca/decla/g
s/Preceeding/Preceding/g

3464–3483 ↗(On Diff #391708)

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.

clang/include/clang/Format/Format.h
2504–2505 ↗(On Diff #391708)

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.

3130–3136 ↗(On Diff #391708)

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.

clang/lib/Format/Format.cpp
1213 ↗(On Diff #391708)

+1 RequiresClausePosition=OwnLine, but also IndentRequires=true.

clang/unittests/Format/FormatTest.cpp
22028–22031

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.

22114–22118

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).

22159

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?

22214

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.

22280–22282

Should these lines be indented because requires is indented, or not?

22280–22282

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.)

22311

s/tyename/typename/g

22377–22379

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.)

22833–22835

(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.

HazardyKnusperkeks marked 4 inline comments as done.Dec 3 2021, 4:19 PM
HazardyKnusperkeks added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3464–3483 ↗(On Diff #391708)

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.

clang/include/clang/Format/Format.h
3130–3136 ↗(On Diff #391708)

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?

clang/unittests/Format/FormatTest.cpp
22028–22031

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.

22114–22118

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.

22159

Take a look at D113369 which will most likely land simultaneously with this change, but currently needs to be updated.

22214

See above.

22833–22835

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.)

Quuxplusone added inline comments.Dec 4 2021, 7:14 AM
clang/docs/ClangFormatStyleOptions.rst
3464–3483 ↗(On Diff #391708)

it was really easy to add

Even easier not to add. ;)

clang/include/clang/Format/Format.h
3130–3136 ↗(On Diff #391708)

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.

clang/unittests/Format/FormatTest.cpp
22028–22031

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.)

22114–22118

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. :)

22159

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.)

22833–22835

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.