Remove use of addAtMain is webidl_binder.py #14585
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kripken
reviewed
Jul 2, 2021
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm, but would it make sense to first figure out the possible regression that came up today in the last PR in this area? I mean, landing this now may make the regression harder to diagnose perhaps?
sbc100
changed the title
Collaborator
Author
|
Updated to include the actual fix for #14583 and an improved test for webidl_binder. |
Support for `addAtMain` in application without a main function was removed in #13901 but I missed that fact that webidl_binder.py was doing this. This change replaces `addAtMain` with `addOnInit` and also makes the use of `addAtMain` in applications withoiut a `main` into an error so that the problem (and solution) should be clear. Update webidl binder test to only use sync compilation when needed, so that code in question actually gets tested. This change also reduces code size by not including `addAtMain` at all in applcications without a `main`. Fixes: #14583.
Member
|
See my question from earlier - did we address that? |
Collaborator
Author
|
Yes. Firstly, I found and fixed the regression here in this change (the webidl breakage). Secondly, this change would have made the regression easier to spot since it no longer fails silently. |
kripken
approved these changes
Jul 13, 2021
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Got it, thanks for the extra context!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support for
addAtMainin applications without a main function wasremoved in #13901 but I missed that fact that webidl_binder.py was doing
this.
This change replaces
addAtMainwithaddOnInitand also makes the useof
addAtMainin applications withoiut amaininto an error so thatthe problem (and solution) should be clear.
Update webidl binder test to only use sync compilation when needed, so
that code in question actually gets tested.
This change also reduces code size by not including
addAtMainat allin applications without a
main.Fixes: #14583