[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

Looks great. Just a few more nits.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11280
+def warn_non_aligned_allocation_function : Warning <
+  "found non aligned allocation function for coroutine %0">,
+  InGroup<CoroNonAlignedAllocationFunction>;
----------------
"under `-fcoro-aligned-allocation`, the non-aligned allocation function for the promise type %0 has higher precedence than the global aligned allocation function"


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11283
+def err_conflicting_aligned_options : Error <
+  "conflicting option '-fcoro-aligned-allocation' and '-fno-aligned-allocation'"
 >;
----------------
ChuanqiXu wrote:
> ychen wrote:
> > Since we're digressing from the usual operator new/delete look-up rules per Option 2, I think it might be easier to just *not* respect `-fno-aligned-allocation` and `-fno-sized-deallocation`. Using '-fcoro-aligned-allocation' explicitly should permit us to assume these functions could be found.
> I guess it may not be good to enable `-fsized-deallocation` automatically if `-fcoro-aligned-allocation` enabled. Since it looks like there are other reasons why we disabled `-fsized-deallocation` before. And it looks it will block our users to use `-fcoro-aligned-allocation`. For the `-faligned-allocation` flag, this is enabled by default but people could disable it explicitly. So the check here is for that.
You're right. It is better to diagnose the conflicting intent.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1425
+      if (HavePlacementArgs)
+        collectPlacementArgs(S, FD, Loc, PlacementArgs);
+      LookupAllocationFunction(/*NewScope*/ Sema::AFS_Class,
----------------
Micro-optimization: instead of recomputing this, add a flag `bool WithoutPlacementArgs` to LookupAllocationFunction to switch between an empty vector and `PlacementArgs`.


================
Comment at: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp:94
+    void return_value(int) {}
+    void operator delete(void *ptr, std::align_val_t);
+  };
----------------
ChuanqiXu wrote:
> ychen wrote:
> > Please add test cases for 
> > ```
> > void operator delete  ( void* ptr, std::size_t, std::align_val_t);
> > void operator delete  ( void* ptr, std::size_t);
> > void operator delete  ( void* ptr);
> > void ::operator delete  ( void* ptr, std::size_t, std::align_val_t);
> > void ::operator delete  ( void* ptr, std::size_t);
> > void ::operator delete  ( void* ptr);
> > ```
> > in that lookup order, and makes sure the look-up order is expected.
> I've tried my best. But it looks hard to test these operator delete under global namespace since they are automatically declared by the compiler.
Thanks for adding these. It looks great! I wouldn't worry about the global ones since this patch did not change their look-up order. 


================
Comment at: clang/test/SemaCXX/coroutine-alloc-4.cpp:49
+    void return_value(int) {}
+    void *operator new(std::size_t, std::align_val_t) noexcept;
+    void *operator new(std::size_t) noexcept;
----------------
ChuanqiXu wrote:
> ychen wrote:
> > Like this test case, please add additional test cases to check the expected look-up order, one test for each consecutive pair should be good.
> > 
> > ```
> > void* T::operator new  ( std::size_t count, std::align_val_t al, user-defined-args... );
> > void* T::operator new  ( std::size_t count, std::align_val_t al);
> > void* T::operator new  ( std::size_t count, user-defined-args... );
> > void* T::operator new  ( std::size_t count);
> > void* operator new  ( std::size_t count, std::align_val_t al );
> > ```
> > 
> > 
> Yeah, I'm testing this in CodeGenCoroutines. (It is hard to test the selection in Sema Test)
Thanks for adding the overload. I think the `noexcept` on operator new is not necessary. Strictly speaking, it is not a conforming API.


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

https://reviews.llvm.org/D133341