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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
