Skip to content
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

Allow for dynamic properties in Object #69

Merged
merged 2 commits into from
Oct 2, 2019

Conversation

Dinnerbone
Copy link
Contributor

Nothing uses them yet, but we will need to change any versioned global method to use it.

Opening a PR before then in case anyone else wants to go make that change, and to avoid conflicts in Object

@Herschel
Copy link
Member

Herschel commented Oct 2, 2019

Thank you! Instead of using NativeFunction, do you think we should have a type Getter = fn(...) -> Value<'gc> and type Setter = Option<fn(..., Value<'gc>)> for Property::Virtual { get, set } ?

@Dinnerbone
Copy link
Contributor Author

I imagine it'll be Executable<'gc> once #63 is in. It will be either a native or user defined function, so the typedef would be type Getter = Executable<'gc> and type Setter = Option<Executable<'gc> - I'm not sure that'll be beneficial.

@Dinnerbone
Copy link
Contributor Author

One other thing: I actually disagree with the clippy lint about preferring to using .. for the debug method as it's deliberate that get is ignored, and it should error if other properties are added (they should likely be printed too).

@Herschel Herschel merged commit 8a43523 into ruffle-rs:master Oct 2, 2019
@Herschel
Copy link
Member

Herschel commented Oct 2, 2019

I imagine it'll be Executable<'gc> once #63 is in.

So then the question is whether to add Exectuable::NativeGetter etc. Not sure it's that beneficial -- my thought is it gets rid of the bounds check when accessing the arg in a setter.

One other thing: I actually disagree with the clippy lint about preferring to using ..

Makes sense! Actually looks like this lint might be removed altogether. I might just allow this lint globally.

Merging, thank you!

Herschel added a commit that referenced this pull request Oct 2, 2019
Often times we want to explicty destructure instead of using ..
because the compiler will emit errors if the structure changes.

(see rust-lang/rust-clippy#1741 and #69)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants