Add embind code size test. NFC #22418
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
brendandahl
reviewed
Aug 20, 2024
tools/emscripten.py
Outdated
|
|
||
| # Decide whether we should generate the global dynCalls dictionary for the dynCall() function? | ||
| if settings.DYNCALLS and '$dynCall' in settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE and len([x for x in exports if x.startswith('dynCall_')]) > 0: | ||
| if settings.DYNCALLS or not settings.WASM_BIGINT or '$dynCall' in settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE: |
There was a problem hiding this comment.
Is this change suppose to be in here?
There was a problem hiding this comment.
Yeah, without this the simple test doesn't compile due to closure warning about dynCalls symbol being missing.
There was a problem hiding this comment.
Actually I found a better way to express this dependency precisely in library.js where it belongs.
sbc100
force-pushed
the
embind_codesize_test
branch
from
40a2464 to
e377bf1
Compare
sbc100
force-pushed
the
embind_codesize_test
branch
from
e377bf1 to
0686d07
Compare
brendandahl
approved these changes
Aug 20, 2024
kripken
reviewed
Aug 20, 2024
|
|
||
| #if DYNCALLS || !WASM_BIGINT | ||
| #if MINIMAL_RUNTIME | ||
| $dynCalls: '[]', |
There was a problem hiding this comment.
This was {} before, and it does seem to be used more like a map than an array - why was this changed?
There was a problem hiding this comment.
Oops you are right... it should be {}
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Aug 20, 2024
This was a mistake from emscripten-core#22418. See emscripten-core#22418 (comment)
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Aug 21, 2024
This was a mistake from emscripten-core#22418. See emscripten-core#22418 (comment)
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Aug 21, 2024
This was a mistake from emscripten-core#22418. See emscripten-core#22418 (comment)
sbc100
added a commit
that referenced
this pull request
Aug 21, 2024
This was a mistake from #22418. See #22418 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In order to make this new test even compile I moved the declaration of
dynCallsfrom being hand coded in python to being decoratively included in library.js, which makes more sense anyway.See #22416