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

shared_ptr<const int> leads to C++ code that doesn't compile #799

Open
bsilver8192 opened this issue Feb 14, 2022 · 4 comments
Open

shared_ptr<const int> leads to C++ code that doesn't compile #799

bsilver8192 opened this issue Feb 14, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@bsilver8192
Copy link
Contributor

bsilver8192 commented Feb 14, 2022

Expected Behavior

Emit bindings that work. dtolnay/cxx#1006 says that inserting a typedef results in cxx::bridge handling it already. dtolnay/cxx#850 talks about making it easier.

dtolnay/cxx#537 and dtolnay/cxx#885 have some discussions around making this type more useful, but that seems like a secondary problem.

Actual Behavior

C++ code with shared_ptr<const int> produces generated C++ that doesn't compile because it tries using shared_ptr<int>. It looks like bindgen is stripping out the necessary information before autocxx has a chance to do anything about it :(.

Steps to Reproduce the Problem

#800 has a test case with int.

Opaque C++ types might need to be handled differently. My code actually uses absl::Span, which might have other issues that require a hand-written intermediate typedef regardless.

Specifications

@bsilver8192
Copy link
Contributor Author

Bleah, not having any luck running that workaround through autocxx either. The typedef gets resolved which strips the const off, and then it has the same problem.

adetaylor added a commit that referenced this issue Feb 14, 2022
@adetaylor
Copy link
Collaborator

It feels to me as though the solution here is to fix dtolnay/cxx#850, and probably the way to fix that is unfortunately to make a whole new equivalent to cxx::SharedPtr, e.g. cxx::SharedPtrConst.

The codebase I'm working with does not tend to use shared_ptr so this is not a very high priority for me - I'm unlikely to work on this.

Here's a proposed phasing for how this could be tackled.

  1. As you note, bindgen does not currently tell us the distinction between shared_ptr<int> and shared_ptr<const int>. This is obviously the first thing to fix. This is probably not something that non-autocxx users of bindgen care about, so we probably need to address this by adding another annotation to our forked bindgen (https://github.com/adetaylor/rust-bindgen). See CppSemanticAttributeCreator.
  2. We should record such attributes as we parse functions within autocxx in src/conversion/parse.
  3. We should detect this within type_converter::confirm_inner_type_is_acceptable_generic_payload, and raise an error, such that such APIs are ignored.
  4. Alternatively/subsequently, in the caller of that function, convert_type_path, we arrange to fall through to the else block for such cases, in which case we'd end up creating a new typedef in C++ automatically, something like std_shared_ptr_int_AutocxxConcrete. So that you'd at least be able to handle these as opaque types within Rust code, though there would be no way to get to the int.
  5. Once cxx has an equivalent of cxx::SharedPtrConst we can switch to using it in these cases.

@bsilver8192
Copy link
Contributor Author

I think the same thing applies to other smart pointer types, like unique_ptr.

I agree that making another set of smart pointer types seems like the way to solve it. Or maybe cxx::SharedPtr/cxx::UniquePtr could use where clauses to implement the appropriate methods/traits for all types?

@bsilver8192
Copy link
Contributor Author

Documenting my workaround for anybody who sees this before it's fixed: I hid the shared_ptr behind a typedef and then used block! on it. For my use case, I don't actually need to call the methods which have this problem, but if I did I think that writing a C++ conversion wrapper would work (use the shared_ptr constructor that takes a shared_ptr of a different type to share the refcount with).

faho added a commit to faho/fish-shell that referenced this issue Apr 11, 2023
This makes function_properties_ref_t not const, in order to work
around cxx - I'm assuming google/autocxx#799
/ dtolnay/cxx#850
faho added a commit to faho/fish-shell that referenced this issue Apr 11, 2023
This makes function_properties_ref_t not const, in order to work
around cxx - I'm assuming google/autocxx#799
/ dtolnay/cxx#850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants