Skip to content

Add #if ASSERTIONS guard around assert in embind_shared #20504

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

Closed
wants to merge 2 commits into from

Conversation

itf
Copy link

@itf itf commented Oct 20, 2023

Adding the #if ASSERTIONS guard fixes the error

ERROR - [JSC_NOT_FUNCTION_TYPE] {
__clutz_actual_namespace: string,
assert: function(*, (Error|string)=): undefined
} expressions are not callable
assert(signature[signature.length - 1] == ")", "Parentheses for argument names should match.");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Collaborator

@brendandahl brendandahl left a comment

Choose a reason for hiding this comment

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

r+ w/ nits fixed

@sbc100
Copy link
Collaborator

sbc100 commented Oct 24, 2023

Do you know why assert is undefined in this case? Can you share the command you run to generate that error?

itf and others added 2 commits October 31, 2023 11:34
Adding the #if ASSERTIONS guard fixes the error

 ERROR - [JSC_NOT_FUNCTION_TYPE] {
  __clutz_actual_namespace: string,
  assert: function(*, (Error|string)=): undefined
} expressions are not callable
        assert(signature[signature.length - 1] == ")", "Parentheses for argument names should match.");
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
apply code review suggestions

Co-authored-by: Brendan Dahl <brendan.dahl@gmail.com>
@itf itf force-pushed the embind-shared-assertion branch from 1c9080b to 6ee793a Compare October 31, 2023 15:34
@itf
Copy link
Author

itf commented Oct 31, 2023

I believe the reason is that I'm compiling with
MINIMAL_RUNTIME=2.

The other flags I'm using are:

PRINTF_LONG_DOUBLE=1
SUPPORT_LONGJMP=emscripten
AUTO_ARCHIVE_INDEXES=0
DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=0
DYNAMIC_EXECUTION=0
MODULARIZE=1
ENVIRONMENT=web,worker
MINIMAL_RUNTIME=2
MINIMAL_RUNTIME_STREAMING_WASM_COMPILATION=1
FILESYSTEM=0
TEXTDECODER=2
POLYFILL=0
ALLOW_MEMORY_GROWTH=1
WASM=1
MAX_WEBGL_VERSION=2

@sbc100
Copy link
Collaborator

sbc100 commented Oct 31, 2023

I'd love to see a test for this.. I'm trying to create a failing test locally, but not having much luck

sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 1, 2023
This matches the behaviour of MINIMAL_RUNTIME, and supports the practice
of not including asserts in release builds.

Also remove the closure externs for `assert` so that closure can
correctly error out end `assert` is not defined.

Helps with emscripten-core#20504
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 1, 2023
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 1, 2023
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 2, 2023
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 2, 2023
This matches the behaviour of MINIMAL_RUNTIME, and supports the practice
of not including asserts in release builds.

Also remove the closure externs for `assert` so that closure can
correctly error out end `assert` is not defined.

Helps with emscripten-core#20504
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 2, 2023
sbc100 added a commit that referenced this pull request Nov 2, 2023
This matches the behaviour of MINIMAL_RUNTIME, and supports the practice
of not including asserts in release builds.

Also remove the closure externs for `assert` so that closure can
correctly error out end `assert` is not defined.

Helps with #20504
sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 2, 2023
sbc100 added a commit that referenced this pull request Nov 3, 2023
@sbc100
Copy link
Collaborator

sbc100 commented Nov 3, 2023

The fix landed in #20606 so I think we can close this one.

@itf itf closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants