Page MenuHomePhabricator

[clang-format] Add better support for co-routinues

Authored by MyDeveloperDay on Dec 1 2021, 2:57 AM.



Responding to a Discord call to help D113977: [Coroutine] Warn deprecated 'std::experimental::coro' uses and heavily inspired by the unlanded D34225: [clang-format] Teach clang-format how to handle C++ coroutines add some support to help coroutinues from not being formatted from

for co_await(auto elt : seq)


co_await(auto elt : seq)

Because of the dominance of clang-format in the C++ community, I don't think we should make it the blocker that prevents users from embracing the newer parts of the standard because we butcher the layout of some of the new constucts.

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Dec 1 2021, 2:57 AM
MyDeveloperDay created this revision.
ChuanqiXu added inline comments.Dec 1 2021, 3:55 AM

It may be worth to add following tests:

co_await co_await co_await foo();
co_await foo().bar();
co_await [this](int a, int b) -> Task {\n
    co_return co_await foo();\n
}(x, y);
co_await [this]() -> Task { co_return x; }

These two statements looks invalid.


I think it would be better to add two tests:

co_return x;
co_return co_await foo();
co_return co_yield foo();
MyDeveloperDay marked 3 inline comments as done.

Add more tests


do you mean these two?

verifyFormat("int x = co_return foo();");
verifyFormat("int x = (co_return foo());");

I lifted these from D34225: [clang-format] Teach clang-format how to handle C++ coroutines

ChuanqiXu added inline comments.Dec 1 2021, 4:53 AM

yeah, since it is meaningless to me since we couldn't evaluate the value of co_return expression.

Quuxplusone accepted this revision.Dec 1 2021, 7:34 AM

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


In fact, you might just say - Improved support for C++20 Modules and Coroutines.


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



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

just to make sure the name int isn't being treated as magic by clang-format


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


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.

This revision is now accepted and ready to land.Dec 1 2021, 7:34 AM
MyDeveloperDay marked 8 inline comments as done.

Address review comments


naming of the tests is to allow easy running of all CoRoutine tests

./FormatTests --gtest_filter=*CoRoutine*


I'd like to add this, sometimes its nice to have tests which might traditionally challenge the code to behave reasonable even if the syntax is invalid


let add both ways around to ensure its behaving.

Quuxplusone added inline comments.Dec 1 2021, 11:16 AM

That's a good rationale for consistency, but the English/C++ word is still "coroutine", not "CoRoutine." If there are other places that need changing, maybe it makes sense to mass-rename in a separate commit. The main thing I'm saying is "CoRoutine" is universally wrong. :)

(I also don't see why *Coroutine* should be a more useful dimension to want to filter on, than *Cpp20* or *Keywords* or *Unary* or whatever. I'd also be surprised if clang-format's tests ever run slow enough that filtering is desired. But I don't know and will defer to you-or-whoever.)

ChuanqiXu accepted this revision.Dec 1 2021, 5:46 PM

LGTM. Thanks!

This revision was landed with ongoing or failed builds.Dec 2 2021, 12:13 AM
This revision was automatically updated to reflect the committed changes.