Page MenuHomePhabricator

[Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type
ClosedPublic

Authored by ChuanqiXu on May 12 2022, 8:41 PM.

Details

Summary

According to https://cplusplus.github.io/CWG/issues/2585.html, this fixes https://github.com/llvm/llvm-project/issues/54881

Simply, the clang tried to found (do lookup and overload resolution. Is there any better word to use than found?) allocation function in promise_type and global scope. However, this is not consistent with the standard. The standard behavior would be that the compiler shouldn't lookup in global scope in case we lookup the allocation function name in promise_type. In other words, the program is ill-formed if there is incompatible allocation function in promise type.

Diff Detail

Event Timeline

ChuanqiXu created this revision.May 12 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 8:41 PM
ChuanqiXu requested review of this revision.May 12 2022, 8:41 PM
ChuanqiXu added inline comments.May 12 2022, 8:42 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11249

The error message here following the wording of GCC.

ChuanqiXu updated this revision to Diff 429176.May 13 2022, 2:39 AM

Remove invalid character

erichkeane added inline comments.
clang/lib/Sema/SemaCoroutine.cpp
1312

Slight preference to just have this as a part of LookupAllocationFunction.

1329

It looks like the NewScope is the only difference between these? I wonder if it might be better off doing the PromiseContainsNew work to just set a variable to THAT and use it later?

ChuanqiXu updated this revision to Diff 429595.May 15 2022, 7:25 PM

Address comments.

ChuanqiXu marked an inline comment as done.May 15 2022, 7:26 PM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaCoroutine.cpp
1312

Yeah, the code would look better in that way. But it would do redundant lookups in that way. So I prefer the current style.

erichkeane accepted this revision.May 16 2022, 6:03 AM
This revision is now accepted and ready to land.May 16 2022, 6:03 AM
This revision was landed with ongoing or failed builds.May 16 2022, 7:37 PM
This revision was automatically updated to reflect the committed changes.
rsmith added a subscriber: rsmith.Jun 3 2022, 12:08 PM
rsmith added inline comments.
clang/lib/Sema/SemaCoroutine.cpp
1312

Nit, should be PromiseContainsNew.

1320

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?

1337

This is no longer correct given the resolution of DR2585: 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.
clang/test/SemaCXX/coroutine-allocs.cpp
22

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.

40

Typo 'actual'

ChuanqiXu marked an inline comment as done.Jun 5 2022, 11:29 PM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaCoroutine.cpp
1320
1337
clang/test/SemaCXX/coroutine-allocs.cpp
22

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.