[cxx-interop] Fix inline operators of address-only types. #32869
Conversation
zoecarver
force-pushed
the
tests/cxx/address-only-operators
branch
6 times, most recently
from
3c646c3 to
d40d337
Compare
zoecarver
changed the title
|
This turned into a patch to actually fix operators for address-only instead of just add tests for them. Should be good to go, now. I may want to add a few more tests, though. |
| @@ -1549,6 +1549,19 @@ class DestructureInputs { | |||
| auto eltPattern = origType.getFunctionParamType(i); | |||
| auto flags = params[i].getParameterFlags(); | |||
|
|
|||
| // If we have an inline operator, when it was imported, it was turned | |||
| // from a method into a static function. So, we need to shift over the | |||
| // params by one and use "self" as the first param's type. | |||
There was a problem hiding this comment.
I can't say what the right fix should be exactly, but I feel like this code is working around the problem more than solving it directly. Resetting NextOrigParamIndex does not feel right to me (so this code won't work for argument count greater than 2, I think?) Abstraction pattern and the code to handle self in this converter class are supposed to abstract away this issue, I think. Currently they handle the abstraction in the opposite direction (importing a C free function as a Swift method). Here, we have the opposite problem -- importing a C++ member function as a Swift static method.
WDYT about extending this code to skip both the Swift self which was mapped from C, and Swift parameters that correspond to C++ this, and letting the HandleForeignSelf handle them at the appropriate point?
There was a problem hiding this comment.
WDYT about extending this code to skip both the Swift self which was mapped from C, and Swift parameters that correspond to C++ this, and letting the HandleForeignSelf handle them at the appropriate point?
The issue is CXXMethodConventions creates a CFunctionTypeConventions object with the function type T(T) when it should be T(T, T). This means that there are the wrong number of arguments. I think we have two choices, either we can pass forSelf=true for the first argument, then decrement NextOrigParamIndex (still a bit of a hack but also less bad than what's currently in this PR), or we can pass a more "correct" type to CFunctionTypeConventions. WDYT would be better?
There was a problem hiding this comment.
I've updated this PR to use the first solution I suggested but I can change it to use the second if you'd rather.
There was a problem hiding this comment.
@hlopko @varungandhi-apple thoughts on the above conversation? I'd like to get this patch moving :)
There was a problem hiding this comment.
@gribozavr @hlopko ping. Once re-reviewed I'll rebase.
zoecarver
force-pushed
the
tests/cxx/address-only-operators
branch
from
d40d337 to
861d368
Compare
zoecarver
force-pushed
the
tests/cxx/address-only-operators
branch
from
861d368 to
304c966
Compare
zoecarver
force-pushed
the
tests/cxx/address-only-operators
branch
from
304c966 to
34802ed
Compare
No description provided.