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

Arthur O'Dwyer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 1 07:34:07 PST 2021

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



More information about the cfe-commits mailing list