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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
