[PATCH] D125517: [Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1312
+
+  bool PromiseContainNew = [this, &PromiseType]() -> bool {
+    DeclarationName NewName =
----------------
Nit, should be `PromiseContainsNew`.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1320
+
+    return !R.empty() && !R.isAmbiguous();
+  }();
----------------
I don't see any tests covering the case where the lookup is ambiguous. Eg, you find `operator new` in two different base classes of the promise type. Please can you add one?


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1337
+
+  LookupAllocationFunction();
 
----------------
This is no longer correct given [the resolution of DR2585](https://wiki.edg.com/pub/Wg21telecons2022/Teleconference2022-06-03/cwg_active.html#2585): the function arguments should only be passed to a class-scope allocation function, not to a global one.

The logic should be:

1) See if there's any class-scope allocation functions. If so, form an argument list based on the function's parameters and try looking for an allocation function. If that succeeds, we're done.
2) Try looking for an allocation function in class scope (if there were any class-scope allocation functions) or in global scope (if not), passing just the size argument.


================
Comment at: clang/test/SemaCXX/coroutine-allocs.cpp:22
+
+resumable f1() { // expected-error {{'operator new' provided by 'std::coroutine_traits<resumable>::promise_type' (aka 'resumable::promise_type') is not usable}}
+  co_return;
----------------
Please can you include a complete error including the "with the function signature of 'f1'" part?

I also wonder if there's some way we can get a candidate list included here.


================
Comment at: clang/test/SemaCXX/coroutine-allocs.cpp:40
+//
+// So the acctual type passed to resumable::promise_type::operator new is lvalue
+// Allocator. It is allowed  to convert a lvalue to a lvalue reference. So the 
----------------
Typo 'actual'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125517