-
Notifications
You must be signed in to change notification settings - Fork 1.5k
C++ Interop: Support getting void* from C++ functions and passing void* it to C++ function
#6279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…void*` it to C++ function This defines `Cpp.void` as a custom type. `Cpp.void*` is mapped to C++ `void*`. Not supported yet: Conversions from and to other pointer types.
toolchain/sem_ir/typed_insts.h
Outdated
| }; | ||
|
|
||
| // A type for C++ `void`. Should only be used for pointers (`void*`). | ||
| struct CustomCppVoidType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's "custom" here -- perhaps remove it?
For contrast, in "custom layout type", I believe "custom" refers to the way layout is determined externally ("customized").
| struct CustomCppVoidType { | |
| struct CppVoidType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
toolchain/sem_ir/stringify.cpp
Outdated
| auto StringifyInst(InstId /*inst_id*/, CustomCppVoidType /*inst*/) -> void { | ||
| *out_ << "<builtin Cpp.void>"; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions here look lexically ordered, perhaps move this before CustomLayoutType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
toolchain/check/cpp/type_mapping.cpp
Outdated
| if (type_id == SemIR::CustomCppVoidType::TypeId && !wrapper_types.empty() && | ||
| context.types().Is<SemIR::PointerType>(wrapper_types.back())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests of things like const (Cpp.void*), (const Cpp.void)*, and Cpp.void**? Particularly the last case looks interesting, would it convert Cpp.void** to void* because wrapper_types.back() is PointerType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this further, (const Cpp.void)* is probably the awkward case. It's essentially a pointer to void, but the const is in a place this code won't like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch!
Added support for const void* and added tests for void** and const void*.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For (const Cpp.void)*, please note that we don't currently support C++ const pointers well.
For example, see #6284.
toolchain/check/cpp/type_mapping.cpp
Outdated
| // If `type_id` is `Cpp.void` and the last wrapper type in `wrapper_types` is a | ||
| // pointer, pops the pointer wrapper and returns non nullable `void*` QualType. | ||
| // Otherwise returns a null QualType. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note, I would've found this comment more helpful if it'd been a little more high level. Right now it seems to be just documenting the if/else in detail. For a suggestion on a different comment style:
| // If `type_id` is `Cpp.void` and the last wrapper type in `wrapper_types` is a | |
| // pointer, pops the pointer wrapper and returns non nullable `void*` QualType. | |
| // Otherwise returns a null QualType. | |
| // Returns `void*` if the type is a wrapped `Cpp.void*`, consuming the | |
| // pointer from `wrapper_types`. Otherwise returns no type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
toolchain/sem_ir/stringify.cpp
Outdated
| } | ||
|
|
||
| auto StringifyInst(InstId /*inst_id*/, CustomCppVoidType /*inst*/) -> void { | ||
| *out_ << "<builtin Cpp.void>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can show up in diagnostics, right? We probably don't want the "<builtin" wrapper as such -- it's a user-visible type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…tomLayoutType`.
toolchain/check/cpp/type_mapping.cpp
Outdated
| llvm::SmallVector<SemIR::TypeId>::iterator pointer_iter; | ||
| if (!wrapper_types.empty() && | ||
| context.types().Is<SemIR::PointerType>(wrapper_types.back())) { | ||
| // `void*`. | ||
| pointer_iter = wrapper_types.end() - 1; | ||
| } else if (wrapper_types.size() >= 2 && | ||
| context.types().Is<SemIR::ConstType>(wrapper_types.back()) && | ||
| context.types().Is<SemIR::PointerType>( | ||
| wrapper_types[wrapper_types.size() - 2])) { | ||
| // `const void*`. | ||
| pointer_iter = wrapper_types.end() - 2; | ||
| } else { | ||
| return clang::QualType(); | ||
| } | ||
|
|
||
| wrapper_types.erase(pointer_iter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative way to write this, which I'm not sure is better:
| llvm::SmallVector<SemIR::TypeId>::iterator pointer_iter; | |
| if (!wrapper_types.empty() && | |
| context.types().Is<SemIR::PointerType>(wrapper_types.back())) { | |
| // `void*`. | |
| pointer_iter = wrapper_types.end() - 1; | |
| } else if (wrapper_types.size() >= 2 && | |
| context.types().Is<SemIR::ConstType>(wrapper_types.back()) && | |
| context.types().Is<SemIR::PointerType>( | |
| wrapper_types[wrapper_types.size() - 2])) { | |
| // `const void*`. | |
| pointer_iter = wrapper_types.end() - 2; | |
| } else { | |
| return clang::QualType(); | |
| } | |
| wrapper_types.erase(pointer_iter); | |
| llvm::SmallVector<SemIR::TypeId>::iterator wrapper_iter = | |
| wrapper_types.end() - 1; | |
| if (context.types().Is<SemIR::PointerType>(*wrapper_iter)) { | |
| // This is `void*`, wrapper_iter points to the pointer type. | |
| } else if (wrapper_iter != wrapper_types.begin() && | |
| context.types().Is<SemIR::ConstType>(*wrapper_iter) && | |
| context.types().Is<SemIR::PointerType>(*(wrapper_iter - 1))) { | |
| // This is `const void*`, move `wrapper_iter` to point to the pointer type. | |
| --wrapper_iter; | |
| } else { | |
| return clang::QualType(); | |
| } | |
| wrapper_types.erase(wrapper_iter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, with some TODO suggestions.
toolchain/check/type_completion.cpp
Outdated
| requires(InstT::Kind.template IsAnyOf< | ||
| SemIR::AssociatedEntityType, SemIR::CppOverloadSetType, | ||
| SemIR::CustomCppVoidType, SemIR::FunctionType, | ||
| SemIR::CppVoidType, SemIR::FunctionType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'd missed this before... Can you please break CppVoidType out to its own function, and add a TODO that it should be always-incomplete?
That is, the changes in this file have the consequence of always treating void as a complete type. However, functionally we don't want it to behave that way -- only pointers are valid. So it should probably never be set as complete.
That should make todo_fail_void.carbon fail, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
FWIW, I was looking into making it always incomplete but didn't find the right setup for this and thought it shouldn't block this change.
Let me know if you have some pointers on how to do that.
toolchain/check/cpp/type_mapping.cpp
Outdated
| llvm::SmallVector<SemIR::TypeId>::iterator pointer_iter; | ||
| if (context.types().Is<SemIR::PointerType>(wrapper_types.back())) { | ||
| // `void*`. | ||
| pointer_iter = wrapper_types.end() - 1; | ||
| } else if (wrapper_types.size() >= 2 && | ||
| context.types().Is<SemIR::ConstType>(wrapper_types.back()) && | ||
| context.types().Is<SemIR::PointerType>( | ||
| wrapper_types[wrapper_types.size() - 2])) { | ||
| // `const void*`. | ||
| pointer_iter = wrapper_types.end() - 2; | ||
| } else { | ||
| return clang::QualType(); | ||
| } | ||
|
|
||
| wrapper_types.erase(pointer_iter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestion to drop pointer_iter (mainly, the type information is complex, but also the direct erase seems helpfully succinct)
| llvm::SmallVector<SemIR::TypeId>::iterator pointer_iter; | |
| if (context.types().Is<SemIR::PointerType>(wrapper_types.back())) { | |
| // `void*`. | |
| pointer_iter = wrapper_types.end() - 1; | |
| } else if (wrapper_types.size() >= 2 && | |
| context.types().Is<SemIR::ConstType>(wrapper_types.back()) && | |
| context.types().Is<SemIR::PointerType>( | |
| wrapper_types[wrapper_types.size() - 2])) { | |
| // `const void*`. | |
| pointer_iter = wrapper_types.end() - 2; | |
| } else { | |
| return clang::QualType(); | |
| } | |
| wrapper_types.erase(pointer_iter); | |
| if (context.types().Is<SemIR::PointerType>(wrapper_types.back())) { | |
| // `void*`. | |
| wrapper_types.pop_back(); | |
| } else if (wrapper_types.size() >= 2 && | |
| context.types().Is<SemIR::ConstType>(wrapper_types.back()) && | |
| context.types().Is<SemIR::PointerType>( | |
| wrapper_types[wrapper_types.size() - 2])) { | |
| // `const void*`. | |
| wrapper_types.erase(wrapper_types.end() - 2); | |
| } else { | |
| return clang::QualType(); | |
| } | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| return clang::QualType(); | ||
|
|
||
| llvm::SmallVector<SemIR::TypeId>::iterator pointer_iter; | ||
| if (context.types().Is<SemIR::PointerType>(wrapper_types.back())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably other instructions that could be here which should be ignored, e.g. MaybeUnformedType. It might be worth thinking about whether a while loop here would be a good fit, but not asking for a change since wrapper_types doesn't support MaybeUnformedType right now either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this definitely makes sense to do when we can have a test that triggers this.
…make it always-incomplete.
This defines
Cpp.voidas a custom type.Cpp.void*is mapped to C++void*.Not supported yet: Conversions from and to other pointer types.
C++ Interop Demo:
Part of #6280.