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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1320
+
+    return !R.empty() && !R.isAmbiguous();
+  }();
----------------
rsmith wrote:
> 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?
Sure. I've addressed this in https://github.com/llvm/llvm-project/commit/448995c521b5ccef71d063bb80f084138ac9d352


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1337
+
+  LookupAllocationFunction();
 
----------------
rsmith wrote:
> 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.
Yeah. This is addressed in https://github.com/llvm/llvm-project/commit/a1ffba8d528681d55c901a997beedbc69946eb90


================
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;
----------------
rsmith wrote:
> 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.
Done. It should be possible and helpful to add a candidate list here. But it might be fine in practice since promise_type wouldn't contain too many allocation functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125517