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.

Sign up for GitHub

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

Jump to bottom

[TheOneric] Enable strict mode in js #46

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

[TheOneric] Enable strict mode in js #46

wants to merge 5 commits into from

Conversation

dmitrylyzo
Copy link
Collaborator

Pulling from the upstream.
Original author: TheOneric

libass#143

@dmitrylyzo dmitrylyzo marked this pull request as draft January 9, 2023 22:01
@dmitrylyzo
Copy link
Collaborator Author

dmitrylyzo commented Jan 10, 2023 •
edited
Loading

This doesn't seem to add "use strict;" to workers - null; has been added instead.
Emscripten 3.1.24 adds "use strict;" to the normal worker, but not to the legacy one.

UPD:
Emscripten 3.1.25 adds "use strict;" to both workers. So this the minimum version for the STRICT_JS feature.

@TheOneric TheOneric mentioned this pull request Jan 11, 2023
Open
18 tasks
@TheOneric
Copy link

Emscripten 3.1.25 adds "use strict;" to both workers. So this the minimum version for the STRICT_JS feature.

STRICT_JS existed since emscripten 1.38.43 (also see introducing commit). So it was supposed to work for much longer, however as you noticed on some versions it didn't, good catch.
It turns out an optimiser pass of emscripten turned "use strict"; int null;, which was fixed with emscripten 3.1.24 (not sure what changed with 3.1.25 or when this bug was first introduced).

@ThaUnknown ThaUnknown mentioned this pull request Feb 15, 2023
Merged
@dmitrylyzo dmitrylyzo force-pushed the TheOneric/strict_js branch from 3252587 to 2fdc2a6 Compare March 27, 2023 19:45
@dmitrylyzo dmitrylyzo mentioned this pull request Feb 29, 2024
Closed
@dmitrylyzo dmitrylyzo force-pushed the TheOneric/strict_js branch from 2fdc2a6 to bab2a72 Compare March 21, 2024 12:16
@dmitrylyzo dmitrylyzo added the enhancement New feature or request label Mar 22, 2024
@dmitrylyzo dmitrylyzo force-pushed the TheOneric/strict_js branch from ab34008 to 5ba02cb Compare March 22, 2024 12:23
TheOneric and others added 5 commits July 23, 2024 16:16
@TheOneric @dmitrylyzo
Enable strict mode in js
14b9606 
This will make some types of bugs easier to detect
and avoid accidental additions of code not working
inside Modules, which always use strict mode.

Performance measurements show no relevant performance
difference between strict and non-strict mode, so this
won't cause regressions in this regard.

Cherry-picked from: libass@c8f02d6
@dmitrylyzo
Update emscripten to 3.1.24
834e8f9 
It turns out an optimizer pass of emscripten turned `"use strict";`
into `null;`, which was fixed with emscripten 3.1.24
@TheOneric @dmitrylyzo
fix EVAL_CTORS
e07f8fd 
EVAL_CTORS now errors when used without WASM,
previosuly it was likely just silently disabled.
Thus, only set it when linking with WASM.

Cherry-picked from: libass@45a05db
@TheOneric @dmitrylyzo
disable closure
a891523 
There's now a new warning when closure isn't set explicitly
for non-WASM builds. Currently, there are errors concerning
redeclaration of the variable screen with closure enabled,
so explicitly disable it.
For the future, we probably want to remove the redeclaration
and enable closure.

Cherry-picked from: libass@45a05db
@dmitrylyzo
Re-add calledMain variable using Module[postRun]
baa99fa 
`calledMain` global variable was removed from the JS runtime
in emscripten 3.1.17
@dmitrylyzo dmitrylyzo force-pushed the TheOneric/strict_js branch from 5ba02cb to baa99fa Compare July 23, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dmitrylyzo @TheOneric