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? |
| @@ -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.
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.
#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.
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.
Additionally, would we like to add a name function too?
There was a problem hiding this comment.
out of curiosity, why is the
argumentsAPI 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.