ChuanqiXu marked 2 inline comments as done. ChuanqiXu added a comment. In D133341#3788283 <https://reviews.llvm.org/D133341#3788283>, @ychen wrote:
> It surprises me that Option 2 does not change > https://eel.is/c++draft/dcl.fct.def.coroutine#10. For consistency, I think it > should. And according to your test case, it deals with alignment as expected. > Probably we should change the P2014R0 wording accordingly. Before that, let's > just mention this difference in the Clang release notes. Yeah, in fact due to the wording of coroutine allocation has changed slightly recently, the wording of P2014 <https://reviews.llvm.org/P2014> must be updated before merged. And I agree it should be consistent. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11283 +def err_conflicting_aligned_options : Error < + "conflicting option '-fcoro-aligned-allocation' and '-fno-aligned-allocation'" >; ---------------- 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. ================ Comment at: clang/lib/Sema/SemaCoroutine.cpp:1302-1318 // According to [dcl.fct.def.coroutine]p9, Lookup allocation functions using a // parameter list composed of the requested size of the coroutine state being // allocated, followed by the coroutine function's arguments. If a matching // allocation function exists, use it. Otherwise, use an allocation function // that just takes the requested size. // // [dcl.fct.def.coroutine]p9 ---------------- ychen wrote: > Update comment here. Since P2014 is not standardized, I feel it might not be good to edit them here. ================ Comment at: clang/lib/Sema/SemaCoroutine.cpp:1400 + // If we found a non-aligned allocation function in the promise_type, + // it indicates the user forgot to update the allocation function. Let's emit ---------------- ychen wrote: > Option 2's order of look-up is > ``` > 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 ); > ``` > Why not allow `void* T::operator new ( std::size_t count);` here? My original thought is to be as strict as we can. But now I feel a warning may be good enough. ================ Comment at: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp:94 + void return_value(int) {} + void operator delete(void *ptr, std::align_val_t); + }; ---------------- 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. ================ 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; ---------------- 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) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133341/new/ https://reviews.llvm.org/D133341 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
