Skip to content
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

Add a function to upcast UniquePtr #1112

Merged
merged 1 commit into from
May 29, 2022

Conversation

bsilver8192
Copy link
Contributor

Upcasting can change the value of a pointer in C++. This means anything
upcasting needs both the parent and child class defined, which is
awkward in user code when wrapping functions that accept ownership of a
superclass object (it requires C++ code that depends on
autocxx-generated C++ code that depends on the main C++ code, which then
needs to be depended on by another iteration of autocxx to call it from
Rust).

Ideally this would be a static function in C++ to avoid some potential
name conflicts that this approach produces (with C++ types sharing a
name in separate namespaces), but I don't think cxx supports those yet
(dtolnay/cxx#1036 and dtolnay/cxx#447).

Upcasting can change the value of a pointer in C++. This means anything
upcasting needs both the parent and child class defined, which is
awkward in user code when wrapping functions that accept ownership of a
superclass object (it requires C++ code that depends on
autocxx-generated C++ code that depends on the main C++ code, which then
needs to be depended on by another iteration of autocxx to call it from
Rust).

Ideally this would be a static function in C++ to avoid some potential
name conflicts that this approach produces (with C++ types sharing a
name in separate namespaces), but I don't think cxx supports those yet
(dtolnay/cxx#1036 and dtolnay/cxx#447).
@adetaylor
Copy link
Collaborator

Thanks, this is great!

One question before I review it in detail.

Ideally this would be a static function in C++ to avoid some potential
name conflicts that this approach produces (with C++ types sharing a
name in separate namespaces), but I don't think cxx supports those yet

This is true, but autocxx does support them by making C++ wrapper functions. So you could do that too, if you felt it were worth it. I suspect that we'd just run into similar name conflict risks though.

@bsilver8192
Copy link
Contributor Author

Ideally this would be a static function in C++ to avoid some potential
name conflicts that this approach produces (with C++ types sharing a
name in separate namespaces), but I don't think cxx supports those yet

This is true, but autocxx does support them by making C++ wrapper functions. So you could do that too, if you felt it were worth it. I suspect that we'd just run into similar name conflict risks though.

That's referring to the upcast wrapper function. I think using the same naming convention as wrappers for static functions would avoid name conflicts on the subclass type. However, now that I think about it, the same problem happens on the superclass type, which is different than static functions because the C++ name needs to be unique (vs static functions already have a class name that's unique within the namespace to use as part of their name). So it's actually more complicated to fully solve than I was thinking.

Do you think it's worth worrying about right now? If so, could you point me to the logic for naming static function wrappers?

@adetaylor
Copy link
Collaborator

I think this is fine as-is for now. Thanks again.

@adetaylor adetaylor merged commit 4f68a2e into google:main May 29, 2022
@bsilver8192 bsilver8192 deleted the subclass-upcast-uniqueptr branch May 29, 2022 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants