Skip to content

/ servo

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

Fixed compile error in generated code, when webidl constructors have same number of args #9786

Merged
merged 1 commit into from Mar 4, 2016
Merged

Fixed compile error in generated code, when webidl constructors have same number of args #9786

merged 1 commit into from Mar 4, 2016

Conversation

@peterjoel
Copy link
Contributor

peterjoel commented Feb 28, 2016

One of the ways that generated code differentiates constructors is by comparing if the args are array-like. The generated code was calling a function IsArrayLike that no longer exists. I re-implemented it with a more rust-like naming scheme.

Review on Reviewable
@highfive
Copy link

highfive commented Feb 28, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@peterjoel peterjoel force-pushed the peterjoel:fix_codegen_is_array_like branch from 38f6543 to 0a8459b Feb 28, 2016
@nox
Copy link
Member

nox commented Feb 28, 2016

Why don't we just call JS_IsArrayObject? This is_array_like function does not seem safe to me, given we can pass an arbitrary *mut JSContext pointer. I know this is a recurrent thing in Servo but this does not look good to me.
@peterjoel
Copy link
Contributor Author

peterjoel commented Feb 28, 2016

@nox I don't understand the difference in terms of safety (one function is just a proxy to the other), but I'll agree that it would be less code to call JS_IsArrayObject directly from the generated code.
@peterjoel peterjoel force-pushed the peterjoel:fix_codegen_is_array_like branch 2 times, most recently from 69f6ebc to 1dd72f7 Feb 28, 2016
@emilio
Copy link
Member

emilio commented Feb 28, 2016

Humm... I'd rather keep is_array_like, in order to keep a TODO comment about how it should change when sequences are implemented correctly. I think this call is going to be difficult to catch later otherwise.
@jdm
Copy link
Member

jdm commented Feb 28, 2016

I agree with @ecoal95. Please also file a follow-up issue to see what Gecko does and copy that.
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 28, 2016

Presumably @@iterator, for which we have a bug already.
@nox
Copy link
Member

nox commented Feb 28, 2016

@ecoal95 @jdm Ok but is_array_like should be unsafe nonetheless.
@peterjoel
Copy link
Contributor Author

peterjoel commented Feb 29, 2016

So the consensus is that I should put it back to how it was, with a separate is_array_like function, but marked unsafe?
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 29, 2016

I'm fine with this.

r? @jdm
@highfive highfive assigned jdm and unassigned Ms2ger Feb 29, 2016
@peterjoel peterjoel force-pushed the peterjoel:fix_codegen_is_array_like branch 2 times, most recently from e7cbf3c to a7696c4 Feb 29, 2016
@jdm
Copy link
Member

jdm commented Mar 2, 2016

@peterjoel: One last request - could you add whatever is necessary to trigger this to TestBinding.webidl, so we have some confidence that the result compiles correctly? :)
@jdm jdm added S-needs-tests and removed S-awaiting-review labels Mar 2, 2016
@peterjoel
Fixed compile error in generated code, when webidl constructors have …
Loading status checks…
3e78b54 
…same number of args

Edited test webidl to show issue, and fix
@peterjoel peterjoel force-pushed the peterjoel:fix_codegen_is_array_like branch from a7696c4 to 3e78b54 Mar 3, 2016
@peterjoel
Copy link
Contributor Author

peterjoel commented Mar 3, 2016

@jdm Done.
@KiChjang
Copy link
Member

KiChjang commented Mar 3, 2016

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Mar 3, 2016

📌 Commit 3e78b54 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Mar 3, 2016

⌛ Testing commit 3e78b54 with merge 8a5c622...
bors-servo added a commit that referenced this pull request Mar 3, 2016
@bors-servo
Auto merge of #9786 - peterjoel:fix_codegen_is_array_like, r=jdm
8a5c622 
Fixed compile error in generated code, when webidl constructors have same number of args

One of the ways that generated code differentiates constructors is by comparing if the args are array-like. The generated code was calling a function `IsArrayLike` that no longer exists. I re-implemented it with a more rust-like naming scheme.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9786)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 4, 2016

💔 Test failed - linux-rel
@emilio
Copy link
Member

emilio commented Mar 4, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Mar 4, 2016

⌛ Testing commit 3e78b54 with merge c37a086...
bors-servo added a commit that referenced this pull request Mar 4, 2016
@bors-servo
Auto merge of #9786 - peterjoel:fix_codegen_is_array_like, r=jdm
Loading status checks…
c37a086 
Fixed compile error in generated code, when webidl constructors have same number of args

One of the ways that generated code differentiates constructors is by comparing if the args are array-like. The generated code was calling a function `IsArrayLike` that no longer exists. I re-implemented it with a more rust-like naming scheme.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9786)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 4, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 3e78b54 into servo:master Mar 4, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@peterjoel peterjoel deleted the peterjoel:fix_codegen_is_array_like branch Aug 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-merge S-needs-tests
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@peterjoel @highfive @nox @emilio @jdm @Ms2ger @KiChjang @bors-servo
You can’t perform that action at this time.