[libcxx-commits] [PATCH] D113977: [Coroutine] Warn deprecated 'std::experimental::coro' uses

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

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.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11015-11017
+  "Found deprecated std::experimental::%0. Consider to update your libc++ "
+  "or move coroutine components into std namespace in case you are using "
+  "self-defined coroutine components">,
----------------
Quuxplusone wrote:
> ldionne wrote:
> > 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.
> +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.
Nit: `s/Warning </Warning</`


================
Comment at: clang/test/SemaCXX/co_await-range-for-exp-namespace.cpp:54
   for
-    co_await(auto i : arr) {}
+    co_await(auto i : arr) {} // expected-warning {{Found deprecated std::experimental}}
   // expected-error at -1 {{call to deleted member function 'await_transform'}}
----------------
ChuanqiXu wrote:
> Quuxplusone wrote:
> > 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) {}
> > ```
> Oh, clang-format couldn't recognize the new grammar. I need to be careful to use it to format codes.
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.)


================
Comment at: libcxx/test/libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp:10
 // REQUIRES: fcoroutines-ts
-// ADDITIONAL_COMPILE_FLAGS: -fcoroutines-ts
+// ADDITIONAL_COMPILE_FLAGS: -fcoroutines-ts -Wno-coroutine
 
----------------
Doesn't the change in lit.local.cfg make this change redundant?


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

https://reviews.llvm.org/D113977