-
Notifications
You must be signed in to change notification settings - Fork 234
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 support for factory functions and static methods. #384
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,7 +181,7 @@ mod filters { | |
Type::Boolean => format!("(True if {} else False)", nm), | ||
Type::String => format!("{}.consumeIntoString()", nm), | ||
Type::Enum(name) => format!("{}({})", class_name_py(name)?, nm), | ||
Type::Object(_) => panic!("No support for lifting objects, yet"), | ||
Type::Object(name) => format!("liftObject({}, {})", class_name_py(name)?, nm), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm mildly surprised that this is a name and not an already created instance, and also a bit confused about how this is "used to support factory functions and methods" as the comment in |
||
Type::CallbackInterface(_) => panic!("No support for lifting callback interfaces, yet"), | ||
Type::Error(_) => panic!("No support for lowering errors, yet"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated to this patch, but the "lowering" here looks like copy-pasta? |
||
Type::Record(_) | Type::Optional(_) | Type::Sequence(_) | Type::Map(_) => format!( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Miscellaneous "private" helpers. | ||
# | ||
# These are things that we need to have available for internal use, | ||
# but that we don't want to expose as part of the public API classes, | ||
# even as "hidden" methods. | ||
|
||
{% if ci.iter_object_definitions().len() > 0 %} | ||
def liftObject(cls, handle): | ||
"""Helper to create an object instace from a handle. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: 'instace'. |
||
|
||
This bypasses the usual __init__() logic for the given class | ||
and just directly creates an instance with the given handle. | ||
It's used to support factory functions and methods, which need | ||
to return instances without invoking a (Python-level) constructor. | ||
""" | ||
obj = cls.__new__(cls) | ||
obj._handle = handle | ||
return obj | ||
{% endif %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: trailing newline |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,24 @@ def __del__(self): | |
) | ||
|
||
{% for meth in obj.methods() -%} | ||
{%- if meth.is_static() -%} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super jazzed about the way this creates four different method implementation blocks (static void, static non-void, non-static void, non-static non-void) but I also don't have a suggestion for how to do it in a neater way. I was hoping that I'd be able to just do like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Macros and includes have got us a long way, but at some point, I expect we'll be embedding templates within templates. |
||
{%- match meth.return_type() -%} | ||
|
||
{%- when Some with (return_type) -%} | ||
@staticmethod | ||
def {{ meth.name()|fn_name_py }}({% call py::arg_list_decl(meth) %}): | ||
{%- call py::coerce_args_extra_indent(meth) %} | ||
_retval = {% call py::to_ffi_call(meth) %} | ||
return {{ "_retval"|lift_py(return_type) }} | ||
|
||
{%- when None -%} | ||
@staticmethod | ||
def {{ meth.name()|fn_name_py }}({% call py::arg_list_decl(meth) %}): | ||
{%- call py::coerce_args_extra_indent(meth) %} | ||
{% call py::to_ffi_call(meth) %} | ||
{% endmatch %} | ||
|
||
{%- else -%} | ||
{%- match meth.return_type() -%} | ||
|
||
{%- when Some with (return_type) -%} | ||
|
@@ -27,4 +45,5 @@ def {{ meth.name()|fn_name_py }}(self, {% call py::arg_list_decl(meth) %}): | |
{%- call py::coerce_args_extra_indent(meth) %} | ||
{% call py::to_ffi_call_with_prefix("self._handle", meth) %} | ||
{% endmatch %} | ||
{% endif %} | ||
{% endfor %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,23 @@ | ||
|
||
public protocol {{ obj.name() }}Protocol { | ||
{% for meth in obj.methods() -%} | ||
{%- if ! meth.is_static() %} | ||
func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_protocol(meth) %}) {% call swift::throws(meth) -%} | ||
{%- match meth.return_type() -%} | ||
{%- when Some with (return_type) %} -> {{ return_type|type_swift -}} | ||
{%- else -%} | ||
{%- endmatch %} | ||
{%- endif %} | ||
{% endfor %} | ||
} | ||
|
||
public class {{ obj.name() }}: {{ obj.name() }}Protocol { | ||
private let handle: UInt64 | ||
|
||
init(fromRawHandle handle: UInt64) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: reduce visibility, perhaps. |
||
self.handle = handle | ||
} | ||
|
||
{%- for cons in obj.constructors() %} | ||
public init({% call swift::arg_list_decl(cons) -%}) {% call swift::throws(cons) %} { | ||
self.handle = {% call swift::to_ffi_call(cons) %} | ||
|
@@ -24,8 +30,27 @@ public class {{ obj.name() }}: {{ obj.name() }}Protocol { | |
} | ||
} | ||
|
||
// TODO: Maybe merge the two templates (i.e the one with a return type and the one without) | ||
static func lift(_ handle: UInt64) throws -> {{ obj.name()|class_name_swift }} { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: reduce visibility. |
||
{{ obj.name()|class_name_swift }}(fromRawHandle: handle) | ||
} | ||
|
||
{# // TODO: Maybe merge the two templates (i.e the one with a return type and the one without) #} | ||
{% for meth in obj.methods() -%} | ||
{%- if meth.is_static() %} | ||
{%- match meth.return_type() -%} | ||
|
||
{%- when Some with (return_type) -%} | ||
public static func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} -> {{ return_type|type_swift }} { | ||
let _retval = {% call swift::to_ffi_call(meth) %} | ||
return {% call swift::try(meth) %} {{ "_retval"|lift_swift(return_type) }} | ||
} | ||
|
||
{%- when None -%} | ||
public static func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} { | ||
{% call swift::to_ffi_call(meth) %} | ||
} | ||
{%- endmatch %} | ||
{%- else -%} | ||
{%- match meth.return_type() -%} | ||
|
||
{%- when Some with (return_type) -%} | ||
|
@@ -39,5 +64,6 @@ public class {{ obj.name() }}: {{ obj.name() }}Protocol { | |
{% call swift::to_ffi_call_with_prefix("self.handle", meth) %} | ||
} | ||
{%- endmatch %} | ||
{%- endif %} | ||
{% endfor %} | ||
} |
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.
I have not attempted to implement static or factory methods for the
gecko_js
backend.Naively, I don't see why they wouldn't work, since we're hewing pretty closely to standard WebIDL here.
But it's not clear to me what is the best stategic use of our collective time at the moment - to keep the gecko_js backend up-to-date as new features are added to uniffi, or to allow it to go a bit stale and bring it up to speed in a burst when we come to use it for an integration in earnest.
@jhugman @dmose thoughts?
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.
I don't want to step on Dan's toes here, but I support letting it go a bit stale in anticipation of that burst happening sooner rather than later.