[PATCH] D114859: [clang-format] Add better support for co-routinues

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM % comments. I agree with all the formatting decisions shown in the new tests.



================
Comment at: clang/docs/ReleaseNotes.rst:270
 
+- Improved Coroutinues support.
+
----------------
s/nues/nes/
In fact, you might just say `- Improved support for C++20 Modules and Coroutines.`


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2957
 
+  // Coroutines
+  if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && Left.Previous &&
----------------
The comment style on line 2953 seems strictly more useful to me. Is it correct to say
```
// co_await (x), co_yield (x), co_return (x)
```
and before line 2961,
```
// co_return;
```
FYI, `co_await;` and `co_yield;` are not currently valid C++; but I agree with the formatting you've chosen here. Please add a test for `"co_await;"` and `"co_yield;"` just to make sure it doesn't regress (even though it's invalid C++20).
Aside: I could even see `co_yield;` one day becoming valid C++ ("yielding void" to its awaiter).


================
Comment at: clang/unittests/Format/FormatTest.cpp:22713
 
+TEST_F(FormatTest, CoRoutineawaitForRangeCpp11) {
+  FormatStyle Style = getLLVMStyle();
----------------
s/CoRoutine/Coroutine/g
Nits:
- I'd just call this function `CoroutineForCoawait`
- Why `"for co_await (auto x : range())\n  ;"` as one string on line 22715, but then split strings on lines 22716-22718 and 22719-22721? Wouldn't it look simpler to use e.g. `"for co_await (auto i : arr) {\n}"` on line 22719?
- Line 22716 is not using `co_await` — is this deliberate or accidental?
- It might make sense to put all `for`-related tests close together in the test file — `for`, `for co_await`, `for constexpr`, `template for`, whatever else we come up with. I don't know if there are any tests for anything but vanilla `for` at the moment.


================
Comment at: clang/unittests/Format/FormatTest.cpp:22727
+
+TEST_F(FormatTest, CoRoutineawait) {
+  verifyFormat("int x = co_await foo();");
----------------



================
Comment at: clang/unittests/Format/FormatTest.cpp:22731
+  verifyFormat("co_await (42);");
+  verifyFormat("void operator co_await(int);");
+  verifyFormat("co_await a;");
----------------
just to make sure the name `int` isn't being treated as magic by clang-format


================
Comment at: clang/unittests/Format/FormatTest.cpp:22756
+  verifyFormat("co_yield 42;");
+  verifyFormat("co_yield n++;");
+}
----------------
IIRC, before we lexed `co_yield` as a keyword, we used to do `co_yield++ n;`. I don't see any way for `co_yield n++;` to get misformatted, though: `co_yieldn++;` would obviously never happen.


================
Comment at: clang/unittests/Format/FormatTest.cpp:22748-22749
+TEST_F(FormatTest, CoRoutinereturn) {
+  verifyFormat("int x = co_return foo();");
+  verifyFormat("int x = (co_return foo());");
+  verifyFormat("co_return (42);");
----------------
ChuanqiXu wrote:
> MyDeveloperDay wrote:
> > ChuanqiXu wrote:
> > > These two statements looks invalid.
> > do you mean these two?
> > 
> > ```
> > verifyFormat("int x = co_return foo();");
> > verifyFormat("int x = (co_return foo());");
> > ```
> > 
> > I lifted these from {D34225}
> yeah, since it is meaningless to me since we couldn't evaluate the value of `co_return` expression.
I agree with @ChuanqiXu: `int x = co_return foo();` is invalid and unrealistic syntax. As usual, I'm ambivalent on whether it makes sense to test invalid inputs (since we still don't want to //crash// on them).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114859/new/

https://reviews.llvm.org/D114859