[PATCH] D126187: [C++20] [Coroutines] Conform the updates for CWG issue 2585

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1293
   // that just takes the requested size.
-
-  FunctionDecl *OperatorNew = nullptr;
-  FunctionDecl *OperatorDelete = nullptr;
-  FunctionDecl *UnusedResult = nullptr;
-  bool PassAlignment = false;
-  SmallVector<Expr *, 1> PlacementArgs;
-
+  //
   // [dcl.fct.def.coroutine]p9
----------------
erichkeane wrote:
> Extra comment line.
Oh, this is intended. I feel the format looks better with the blank line.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1355
+  // We don't expect to call to global operator new with (size, p0, …, pn).
+  if (PromiseContainNew && !collectPlacementArgs(S, FD, Loc, PlacementArgs))
+    return false;
----------------
erichkeane wrote:
> Can you explain how this works?  I'm not seeing what part of the collection of placement args would prohibit the call you're talking about.
The new version of CWG issue 2585 says: "We shouldn't generate an allocation call in global scope with (std::size_t, p0, ..., pn)". Also it says that we wouldn't lookup into global scope if we find `operator new` in promise_type. It implies that we should only generate an allocation call in promise_type scope   with (std::size_t, p0, ..., pn). So we should only collect placement arguments if we saw `operator new` in promise_type scope.

In other words, if we don't add the check, it would collect placement arguments all the way so it would be possible to generate an allocation call in global scope with (std::size_t, p0, ..., pn), which violates the update of CWG issue 2585.


================
Comment at: clang/test/SemaCXX/coroutine-allocs2.cpp:2
+// Tests that we wouldn't generate an allocation call in global scope with (std::size_t, p0, ..., pn)
+// Although this test generates codes, it aims to test the semantics. So it is put here.
+// RUN: %clang_cc1 %s -std=c++20 -S -triple x86_64-unknown-linux-gnu -emit-llvm -disable-llvm-passes %s -o - | FileCheck %s
----------------
erichkeane wrote:
> I'm not a fan at all of having a FIleCheck-type-test in Sema, and this DOES validate semantics.  Is there any way to get this to emit an error instead?  Perhaps declare the generated operator-new as 'deleted' and show that it chooses THAT one instead by an error?
I tried to mark the allocation function as delete. But it doesn't work... And FileCheck is the only way I imaged. Another possible way might be to use FileCheck for dumped AST. But I feel it is worse since the ABI is more stable than the AST. (The format of AST is not guaranteed stable I thought.)


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

https://reviews.llvm.org/D126187