Calling overloaded C++ operators from Rust #1904

Volker-Weissmann · 2020-10-18T01:07:05Z

What I did in this PR can be best described using the "bintest" test that I created:
The following works now (See "bintest" folder for full source, of the testcase run "Cargo run" in the "bintest" folder for a proof that it works.):

fn main() {
	let mut a = MyClass{val: 27, other:0};
	let b = MyClass{val: 13, other:0};
	a += &b; // should call the overloaded C++ operator+=
	let a = &a + &b; // should call the overloaded C++ operator+
	let a = -&a; // should call the overloaded C++ operator- (unary)
	let a = &a - &b; // should call the overloaded C++ operator- (binary)
	assert!(a == MyClass{val: -66, other:0}); // should call the overloaded C++ operator==
}

class MyClass {
public:
    int val;
    int other;
    MyClass operator+(const MyClass& rhs) const;
    MyClass operator-(const MyClass& rhs) const;
    MyClass& operator+=(const MyClass& rhs);
    bool operator==(const MyClass& rhs) const ;
    int operator[](size_t index) const;
    MyClass operator-() const;
};

Up next, I will try to fix #1840 #1841

Note: This is my second open source contribution that is not just a bugfix/documentation fix. Thank you for writing easy to read code.

…ators

…to master

emilio

Exciting! I need to look a bit more in detail but here's some feedback from a quick look.

Thanks for working on this!

emilio · 2020-10-18T15:39:59Z

src/ir/context.rs

+            ("[]", "_index"),
+            ("!", "_not"),
+        ];
+        let mut invalid_name = false;


nit: let invalid_name = subs.iter().any(|sub| name.contains(sub.0)).

Fixed in the next commit

emilio · 2020-10-18T15:42:14Z

src/codegen/mod.rs

+                let trait_name = proc_macro2::TokenStream::from_str(el.1).unwrap();
+                let func_name = proc_macro2::TokenStream::from_str(el.2).unwrap();
+                result.push(quote!(
+                    impl std::ops::#trait_name<#rhs_type> for &#ty_for_impl {


This needs to preserve the trait_prefix, to handle use_core correctly.

Fixed in the next commit

emilio · 2020-10-18T15:43:12Z

src/codegen/mod.rs

+            None
+        } else {
+            dbg!("test");
+            let temp = utils::type_id_to_rust_type(ctx, signature.argument_types()[1].1);


Can't we dig here if there is a pointer instead here?

What do you mean with "digging"?

emilio · 2020-10-18T15:43:48Z

src/codegen/mod.rs

+    pub fn type_id_to_rust_type(
+        ctx: &BindgenContext,
+        ty: crate::ir::context::TypeId,
+    ) -> proc_macro2::TokenStream {


It seems it'd be doable here to add a flag to convert pointers to references, but maybe I'm missing something? Seems better than fixing up the token-stream afterwards.

Adding a flag to convert pointers to references would be a nice solution, but doing this is tricky, at least for me.

Something like this seems to be relatively straight-forward. Should apply on top of your patch.

diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs index 9eb69f01..00ee92df 100644 --- a/src/codegen/mod.rs +++ b/src/codegen/mod.rs @@ -2173,45 +2173,6 @@ impl CodeGenerator for CompInfo { } } -/// This function takes a TokenStream, removes all "const" and replaces all "*" -/// with "&". -/// -/// This is a bit hacky, but we need it because argument_type_id_to_rust_type -/// returns something like "const *MyClass" instead of "&MyClass" and the latter -/// is what we need to write into bindgen.rs. The less hacky way would be to -/// make argument_type_id_to_rust_type use "&" instead of "*" when assembling -/// the TokenStream, but doing this is tricky. -fn raw_pointer_to_reference( - rust_source: proc_macro2::TokenStream, -) -> proc_macro2::TokenStream { - // Todo: Check if the performance of this function is terrible if rust_source is long. - use proc_macro2::*; - rust_source - .into_iter() - .filter(|elem| match &elem { - TokenTree::Ident(ident) => { - let comp = Ident::new("const", ident.span()); - if comp == ident.clone() { - false - } else { - true - } - } - _ => (true), - }) - .map(|elem| match &elem { - TokenTree::Punct(punct) => { - if punct.as_char() == '*' { - TokenTree::Punct(Punct::new('&', Spacing::Alone)) - } else { - elem - } - } - _ => (elem), - }) - .collect() -} - impl Method { /// If self is an overloaded operator that this function recognizes, this /// function will put the correct wrapper into result and return true. Note @@ -2261,11 +2222,11 @@ impl Method { let second_arg_type = if args.len() < 2 { None } else { - let temp = utils::argument_type_id_to_rust_type( + Some(utils::argument_type_id_to_rust_type( ctx, signature.argument_types()[1].1, - ); - Some(raw_pointer_to_reference(temp)) + /* pointer_to_ref = */ true + )) }; // We then check if the function name is in one of the following three @@ -4617,6 +4578,7 @@ mod utils { pub fn argument_type_id_to_rust_type( ctx: &BindgenContext, ty: crate::ir::context::TypeId, + pointer_to_ref: bool, ) -> proc_macro2::TokenStream { use super::ToPtr; @@ -4641,6 +4603,11 @@ mod utils { stream.to_ptr(ctx.resolve_type(t).is_const()) } TypeKind::Pointer(inner) => { + if pointer_to_ref { + let inner = inner.to_rust_ty_or_opaque(ctx, &()); + return quote! { &#inner } + } + let inner = ctx.resolve_item(inner); let inner_ty = inner.expect_type(); if let TypeKind::ObjCInterface(ref interface) = @@ -4679,7 +4646,7 @@ mod utils { assert!(!arg_name.is_empty()); let arg_name = ctx.rust_ident(arg_name); - let arg_ty = argument_type_id_to_rust_type(ctx, ty); + let arg_ty = argument_type_id_to_rust_type(ctx, ty, /* pointer_to_ref = */ false); quote! { #arg_name : #arg_ty

Seems like a nice solution. Should I commit this patch or do you want to do that?

Feel free to commit it yourself :)

This patch is c9111e3

emilio · 2020-10-18T15:45:15Z

src/codegen/mod.rs

@@ -4411,6 +4616,52 @@ mod utils {
        }
    }

+    pub fn type_id_to_rust_type(


Does this need to be public? Also, I think the name of this function should have something to do with arguments. The conversions done here are only valid for arguments.

If I remove the "pub" it does not compile because try_codegen_operator calls it.
I changed the name to argument_type_id_to_rust_type in the next commit.

emilio · 2020-10-18T15:47:55Z

src/codegen/mod.rs

+                            let retptr = unsafe {
+                                #function_name(self, rhs)
+                            };
+                            assert_eq!(retptr, self as *mut #ty_for_impl, "The bindgen authors have no idea how to handle this case.");


I don't think we necessarily care about asserting this here, do we? As in, the rust trait doesn't have any return value, it's fine to drop it on the floor.

Where C++ thinks the object lives and where Rust thinks the object lives have to be identical. Most C++ operators just return this; , but afaik this is not required by the standard.

Right, my point is that where the object lives is not meaningful is it? Like, a C++ operator can return whatever it wants, but it can't destroy this. So I still don't know why this assert is useful.

I removed this assertion in the newest commit. I hope you are right...

emilio · 2020-10-18T15:48:47Z

bintest/example.cpp

@@ -0,0 +1,41 @@
+#include <cstddef>
+class MyClass {


It seems this could reuse the bindgen-integration test that is set up already, can't it?

I think that'd be preferable.

I did not know that bindgen-integrat existed. Fixed in the newest commit.

This reverts commit d765dff.

emilio · 2020-10-27T11:09:59Z

src/ir/context.rs

+            ("=", "_equal"),
+            ("[]", "_index"),
+            ("!", "_not"),
+        ];


I'm a bit concerned about the perf implications of this, but it should be optimizable relatively easily so no need to dig into that if not wanted.

The old way had the problem that you have to write the reserved symbols twice, a violation of the dry rule.
Is performance even that much of an issue for bindgen?

bors-servo · 2021-01-30T15:50:00Z

☔ The latest upstream changes (presumably c509ef1) made this pull request unmergeable. Please resolve the merge conflicts.

Volker-Weissmann · 2021-01-31T05:03:56Z

I'm abondoning this in favor of https://github.com/google/autocxx .
Probably would not be merged anyway.

emilio · 2021-02-01T01:40:30Z

I still want to get to review this (if you're fine with that). I'm sorry for the massive lag here again.

Volker-Weissmann · 2021-02-04T21:09:28Z

I solved the merge conflicts.

bors-servo · 2021-02-07T22:12:19Z

☔ The latest upstream changes (presumably 8ac787a) made this pull request unmergeable. Please resolve the merge conflicts.

Volker-Weissmann · 2021-09-08T13:10:42Z

If you don't want to merge it, that's fine for me. If you do want to merge it, tell me then I will fix the failing tests and merge conflicts.

bors-servo · 2021-10-27T17:24:06Z

☔ The latest upstream changes (presumably 9738fb9) made this pull request unmergeable. Please resolve the merge conflicts.

pvdrz · 2022-09-22T20:16:48Z

@emilio, do you still want this feature in bindgen? I can rebase this PR if that's the case.

Volker-Weissmann · 2022-09-25T08:15:28Z

@emilio, do you still want this feature in bindgen? I can rebase this PR if that's the case.

Note that this PR might be unneccesary due to the existance of autocxx.

Volker-Weissmann added 6 commits October 18, 2020 02:40

Overloaded C++ operators can now be called as member functions.

b43bedc

Overloaded C++ operators can now be called using overloaded Rust oper…

3909e79

…ators

Added test: Calling overloaded C++ operators from Rust should work.

c8fe526

Merge branch 'master' of https://github.com/rust-lang/rust-bindgen in…

d2e1701

…to master

minor fixes

b7c1ff7

ran rustfmt

a85a2ac

emilio reviewed Oct 18, 2020

View reviewed changes

Volker-Weissmann added 4 commits October 18, 2020 22:57

minor fixes

0c8c2b5

fixed tests

d765dff

Revert "fixed tests"

39a5b95

This reverts commit d765dff.

fixed tests

11f88ed

emilio reviewed Oct 27, 2020

View reviewed changes

Volker-Weissmann added 4 commits October 31, 2020 00:35

Improved code quality by applying patch from [email protected]

c9111e3

Removed (hopefully) unnecessary assertion.

6ee9e85

Merge branch 'master' of https://github.com/rust-lang/rust-bindgen

94b0243

ran cargo fmt

88de490

highfive added the S-awaiting-review label Jan 5, 2021

simple bugfix

dc8af59

highfive added the S-needs-rebase label Jan 30, 2021

Volker-Weissmann closed this Jan 31, 2021

emilio reopened this Feb 1, 2021

Volker-Weissmann added 2 commits February 4, 2021 21:58

Merge branch 'master' into overloaded_operators

ef81c19

merged from master

755dcf6

Volker-Weissmann added 2 commits February 7, 2021 23:27

merging master

d3f0091

temp

c6bf78e

pvdrz deleted the branch rust-lang:master November 2, 2023 17:50

pvdrz closed this Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calling overloaded C++ operators from Rust #1904

Calling overloaded C++ operators from Rust #1904

Volker-Weissmann commented Oct 18, 2020 •

edited

Loading

emilio left a comment

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 27, 2020

Volker-Weissmann Oct 27, 2020

emilio Oct 28, 2020

Volker-Weissmann Oct 30, 2020

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 27, 2020

Volker-Weissmann Oct 30, 2020

emilio Oct 18, 2020

Volker-Weissmann Oct 18, 2020

emilio Oct 27, 2020

Volker-Weissmann Oct 27, 2020

bors-servo commented Jan 30, 2021

Volker-Weissmann commented Jan 31, 2021 •

edited

Loading

emilio commented Feb 1, 2021

Volker-Weissmann commented Feb 4, 2021 •

edited

Loading

bors-servo commented Feb 7, 2021

Volker-Weissmann commented Sep 8, 2021

bors-servo commented Oct 27, 2021

pvdrz commented Sep 22, 2022

Volker-Weissmann commented Sep 25, 2022

Calling overloaded C++ operators from Rust #1904

Calling overloaded C++ operators from Rust #1904

Conversation

Volker-Weissmann commented Oct 18, 2020 • edited Loading

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Jan 30, 2021

Volker-Weissmann commented Jan 31, 2021 • edited Loading

emilio commented Feb 1, 2021

Volker-Weissmann commented Feb 4, 2021 • edited Loading

bors-servo commented Feb 7, 2021

Volker-Weissmann commented Sep 8, 2021

bors-servo commented Oct 27, 2021

pvdrz commented Sep 22, 2022

Volker-Weissmann commented Sep 25, 2022

Volker-Weissmann commented Oct 18, 2020 •

edited

Loading

Volker-Weissmann commented Jan 31, 2021 •

edited

Loading

Volker-Weissmann commented Feb 4, 2021 •

edited

Loading