Allow function aliases #218
Conversation
|
I improved this thing. It allows to import overloaded functions and methods from C++ now. |
There was a problem hiding this comment.
Thanks! This is great.
Two suggestions:
- I am concerned that
alias(name)does not say anything about which is the C++ name and which is the Rust name. The style I would prefer is#[rust_name = "name"]/#[cxx_name = "name"]where every method keeps track of both a Rust name and a C++ name, usually the same, but either can be specified according to what makes most sense to the programmer in context. - Please avoid making changes to the demo directories. Those are supposed to be an introductory example, not to exhaustively exercise all features. Please put tests for this feature in tests/test.rs and tests/ffi.
|
It's done! |
|
Thanks for this @ VKachemtsev! I am looking forward to using it |
| @@ -516,9 +516,10 @@ fn write_cxx_function_shim( | |||
| writeln!(out, ") noexcept {{"); | |||
| write!(out, " "); | |||
| write_return_type(out, &efn.ret); | |||
| let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident }; | |||
There was a problem hiding this comment.
I don't like this implementation of functions having an "ident" and an optional "alias", and especially counting on each use site to remember whether they should be checking for presence of the alias.
In my mind every function should have a non-optional rust name and non-optional c++ name. That's how we should store them post-parsing. Every place which uses of a function name will pick which of the two they want.
| @@ -10,6 +10,7 @@ pub struct Parser<'a> { | |||
| pub doc: Option<&'a mut Doc>, | |||
| pub derives: Option<&'a mut Vec<Derive>>, | |||
| pub repr: Option<&'a mut Option<Atom>>, | |||
| pub alias: Option<&'a mut Option<(Ident, bool)>>, | |||
There was a problem hiding this comment.
This needs a comment indicating what the bool means, or use an enum with two variants instead.
| } else { | ||
| return cx.error(attr, "unsupported attribute"); | ||
| } | ||
| return cx.error(attr, "unsupported attribute"); |
There was a problem hiding this comment.
This was correct before the PR. The change made in the PR makes well-formed attributes silently accepted anywhere where not expected, like putting a #[derive()] attribute on a function or a #[repr()] attribute on a function.
| @@ -101,6 +101,15 @@ pub mod ffi { | |||
| fn set2(&mut self, n: usize) -> usize; | |||
| fn set_succeed(&mut self, n: usize) -> Result<usize>; | |||
| fn get_fail(&mut self) -> Result<usize>; | |||
|
|
|||
There was a problem hiding this comment.
Remove trailing whitespace. (Elsewhere as well)
| Lang::Cxx if !is_rust => { | ||
| return Err(Error::new_spanned(foreign_fn, "C/C++ functions with 'cxx_name' are not allowed")); | ||
| }, | ||
| Lang::Rust if is_rust => { | ||
| return Err(Error::new_spanned(foreign_fn, "Rust functions with 'rust_name' are not allowed")); | ||
| }, |
There was a problem hiding this comment.
I think this is overly restrictive. This would make people need to remember which attribute is legal on which kind of function, and it's not needed -- any function should be able to have either attribute. We'll use the function's "real" name as the one that isn't specified by an attribute.
| let ident = if let Some(alias) = &efn.alias { alias } else { &efn.ident }; | ||
| if !function_names.insert((&efn.receiver, ident)) { | ||
| duplicate_name(cx, efn, ident); | ||
| } |
There was a problem hiding this comment.
As implemented in this PR, the alias of an extern Rust function is its C++ name and the alias of an extern C++ function is its Rust name. So this check isn't correct:
- It is going to have false positives on extern Rust functions having the same C++ name, which we should allow because C++ has function overloading.
- It is going to have false negatives on an extern C++ function with alias f colliding with an extern Rust function named f if the Rust one also has an alias.
- It is going to have false positives on the Rust name of a C++ function being the same as the C++ name of a Rust function, which should in general have no bearing on each other.
Hi! Thank you for cxx, it is a great thing! I have not found any way to rename functions and methods imported from C ++. I suggest an additional attribute 'alias' and an example of use.