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

[embind] Always enable assertions during TypeScript generation. #21695

Merged
merged 6 commits into from
Sep 26, 2024
Merged

[embind] Always enable assertions during TypeScript generation. #21695

merged 6 commits into from
Sep 26, 2024

Conversation

brendandahl
Copy link
Collaborator

Fixes #21641

@brendandahl brendandahl requested a review from sbc100 April 4, 2024 21:58
sbc100
sbc100 reviewed Apr 4, 2024
View reviewed changes
test/test_other.py Outdated Show resolved Hide resolved
@brendandahl brendandahl force-pushed the tsgen-enable-assertions branch from cb06945 to 0483145 Compare April 8, 2024 23:29
@brendandahl brendandahl requested a review from sbc100 April 8, 2024 23:30
sbc100
sbc100 reviewed Sep 24, 2024
View reviewed changes
tools/link.py Outdated
@@ -1966,6 +1966,8 @@ def run_embind_gen(wasm_target, js_syms, extra_settings, linker_inputs):
settings.SINGLE_FILE = False
# Embind may be included multiple times, de-duplicate the list first.
settings.JS_LIBRARIES = dedup_list(settings.JS_LIBRARIES)
# Assertions are always used.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention why?

@brendandahl brendandahl enabled auto-merge (squash) September 24, 2024 21:42
@brendandahl
Copy link
Collaborator Author

Something changed so now when assertions are enabled $FS needs $strError which needs _strerror. _strerror is not exported from the wasm file with assertions disabled. I think previously an assert build didn't have different wasm exports or maybe we weren't testing that.

@brendandahl
Copy link
Collaborator Author

I guess this was #21172. Maybe instead of relying on assert I'll just throw JS exceptions.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 24, 2024

I guess this was #21172. Maybe instead of relying on assert I'll just throw JS exceptions.

Unless you are in strict mode assert should always exist

@brendandahl
Copy link
Collaborator Author

I guess this was #21172. Maybe instead of relying on assert I'll just throw JS exceptions.

Unless you are in strict mode assert should always exist

Yeah, the original issue filed above was using -s STRICT=1.

@brendandahl
Copy link
Collaborator Author

Or I guess I can do ifdefs like all the other library code.

sbc100
sbc100 reviewed Sep 26, 2024
View reviewed changes
src/embind/embind_gen.js
@@ -476,10 +476,9 @@ var LibraryEmbind = {
}
argStart = 2;
}
#if ASSERTIONS
if (argsName.length)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add curly braces around the if body ?

@brendandahl brendandahl merged commit 0aa6ad4 into emscripten-core:main Sep 26, 2024
28 checks passed
@dschuff dschuff mentioned this pull request Oct 29, 2024
Merged
@dschuff dschuff mentioned this pull request Nov 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbc100 sbc100 sbc100 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--embind-emit-tsd may cause "ReferenceError: assert is not defined"
2 participants
@brendandahl @sbc100