-
Notifications
You must be signed in to change notification settings - Fork 230
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 "return_type()" to Constructor #1490
Add "return_type()" to Constructor #1490
Conversation
This makes Constructor more nominal with Function, i.e. the constructor can be used as a function in macros: ``` {% macro(func) %} {{ func.return_type() }} {% endmacro %} {% call macro(constructor) %} ``` This is very useful for generating Go bindings, because in Go errors are handled via multiple return values, instead of exceptions. That means that codegen for function return signatures get quite complicated, and being able to re-use the same code for constructors as for regular functions makes it much easier to generate code.
I was going to mention this when I looked at the C# bindings, but what do you think about adding a |
|
Looks good to me, but I don't want to merge it myself, since that's sort-of like reviewing my own code. @tarikeshaq what do you think of this approach? |
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.
Thank's a ton for contributing back, I'm super excited to see more backends (and a go backends sounds phenomenal!)
Had a few nit but this Makes sense to me as long as it's useful for external backends!
@@ -290,6 +295,9 @@ impl Constructor { | |||
self.ffi_func.name = format!("{ci_prefix}_{obj_name}_{}", self.name); | |||
self.ffi_func.arguments = self.arguments.iter().map(Into::into).collect(); | |||
self.ffi_func.return_type = Some(FfiType::RustArcPtr(obj_name.to_string())); | |||
|
|||
// this is a bit of a dirty place to put this, but there isn't another "general" pass |
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 we expand why this is not an ideal place? Inline comment is great, but if we have a more actionable way of making it more ideal then a bug filed with more context would be phenomenal!
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.
#1469 adds an object_name
field to Constructor
, which would avoid this. Does that make sense for the long-term plan?
@@ -249,6 +249,42 @@ impl APIConverter<Argument> for weedle::argument::SingleArgument<'_> { | |||
} | |||
} | |||
|
|||
/// Implemented by function-like types (Function, Method, Constructor) | |||
pub trait Callable { |
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.
out of curiosity, why is the arguments
API return a vec of references where the other two return owned Type
?
Is it because it was hard to have then all return references without overcomplicating the code?
Just curious!
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.
Additionally, would we like to add a name
function too?
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.
out of curiosity, why is the
arguments
API return a vec of references where the other two return ownedType
?
I think that was copied from the #1469 implementation. I didn't put a lot of thought into it, but that seemed like the path of least resistance to me. In general, it seems like we tend to clone Type rather than try to manage a reference to it. I'm not sure if that's the best policy going forward or not.
Additionally, would we like to add a name function too?
Maybe? I'm not totally sure though, there could be callables that don't have a name. For example a destructor. Also, the primary constructor currently has a name field, but it's just hard coded to "new". That said, I'm not really against it I just didn't put the time into defining it.
#1469 ended up being merged before this one, should we just close this PR? |
Yeap, #1469 looks good to me. |
This makes Constructor more nominal with Function, i.e. the constructor can be used as a function in macros:
This is very useful for generating Go bindings, because in Go errors are handled via multiple return values, instead of exceptions. That means that codegen for function return signatures get quite complicated, and being able to re-use the same code for constructors as for regular functions makes it much easier to generate code.