This is an archive of the discontinued LLVM Phabricator instance.

[Coroutine] Warn deprecated 'std::experimental::coro' uses
ClosedPublic

Authored by ChuanqiXu on Nov 16 2021, 3:24 AM.

Details

Summary

Since we've decided the to not support std::experimental::coroutine*, we should tell the user they need to update.
We could emit warning on the compiler side or in libc++'s side by #warning directives.
I choose to warn on the compiler side since some people might use libstdc++ + self-defined coroutine header (Seastar is an example: https://github.com/scylladb/seastar/blob/master/include/seastar/core/std-coroutine.hh).
Since new warning might break the libcxx's CI system, I add -Wno-coroutine for the legacy tests. I guess it would be OK since the legacy test would be removed.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Nov 16 2021, 3:24 AM
ChuanqiXu requested review of this revision.Nov 16 2021, 3:24 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 16 2021, 3:24 AM
ldionne accepted this revision.Nov 16 2021, 6:43 AM

LGTM with some suggestions, thanks for adding this notice!

clang/include/clang/Basic/DiagnosticGroups.td
57–59
clang/include/clang/Basic/DiagnosticSemaKinds.td
11015–11017

I would simply reword as "Please move from std::experimental::%0 to std::%0. Support for std::experimental::%0 will be removed in LLVM 15."

IMO the set of users defining their own coroutines header is small and it's not worth making the warning more obtuse for that corner case.

libcxx/test/std/experimental/language.support/support.coroutines/lit.local.cfg
8
This revision is now accepted and ready to land.Nov 16 2021, 6:43 AM
Mordante accepted this revision.Nov 16 2021, 7:35 AM
Mordante added a subscriber: Mordante.

Thanks for working on this and landing the coroutines. LGTM after applying @ldionne's suggestions.

Quuxplusone requested changes to this revision.Nov 16 2021, 7:51 AM
Quuxplusone added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11015–11017

+1; people who write their own "coroutine.h" header are also operating outside of standard C++ and outside of the Clang/libc++ ecosystem, so they should be quite used to figuring things out for themselves. If they want friendly help from the toolchain, they should use the toolchain's headers.

clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp
54

I'd like to see the next word in this diagnostic. (Ditto throughout. I'd also like to make sure that the diagnostic is not being printed with single-quotes in the wrong place, e.g. Found deprecated std::experimental::'coroutine_handle'; if it is, please fix that.)

This code is also misindented; please fix it up, as long as you're touching it. Should be

for co_await (auto i : arr) {}
This revision now requires changes to proceed.Nov 16 2021, 7:51 AM
ChuanqiXu updated this revision to Diff 387811.Nov 16 2021, 7:30 PM

Address comments.

ChuanqiXu marked 5 inline comments as done.Nov 16 2021, 7:33 PM
ChuanqiXu added inline comments.
clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp
54

Oh, clang-format couldn't recognize the new grammar. I need to be careful to use it to format codes.

libcxx/test/std/experimental/language.support/support.coroutines/lit.local.cfg
8

Since the option is new added, I wonder if the CI would break if it didn't update compiler in time. (Or if it would run CI for previous compiler). Let's see if the CI would be green.

ChuanqiXu updated this revision to Diff 387828.Nov 16 2021, 9:58 PM
ChuanqiXu marked 2 inline comments as done.

Use -Wno-coroutine instead of -Wno-deprectated-experimental-coroutine to keep CI stable.

The CI fails. But I guess it would be irrelevant with this diff:

/var/lib/buildkite-agent/builds/llvm-project/libcxx/.clang-format:6:17: error: invalid boolean
SpacesInAngles: Leave
                ^~~~~
Error reading /var/lib/buildkite-agent/builds/llvm-project/libcxx/.clang-format: Invalid argument

FWIW, the problem with

SpacesInAngles: Leave
                ^~~~~

is that the debian builder has an outdated (or maybe just 13.x?) clang-format; this option is super new. I don't know what the appropriate course of action is, there.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11015–11017

Nit: s/Warning </Warning</

clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp
54

FYI: I'd prefer a space before the (. Compare for (~~~), for co_await (~~~). (I've left a comment in the clang-format Discord channel, too.)

libcxx/test/libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp
10

Doesn't the change in lit.local.cfg make this change redundant?

This revision was not accepted when it landed; it landed in state Needs Review.Nov 17 2021, 5:43 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
ChuanqiXu marked 2 inline comments as done.Nov 17 2021, 5:57 PM
ChuanqiXu added inline comments.
clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp
54

Thanks for issuing this!

libcxx/test/libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp
10

Yeah, this test isn't covered by lit.local.cfg below.