Skip to content

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 5, 2025

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 5, 2025

@tlively for test_wasm_intrinsics_simd changes

'': ([],),
'unsigned_char': (['-funsigned-char'],),
})
def test_wasm_intrinsics_simd(self, args):
# Improves test readability
Copy link
Member

Choose a reason for hiding this comment

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

This comment goes with the -Wno-c++11-narrowing, but it looks like that flag was mistakenly dropped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this test is not doesn't contain any such warnings.. and its not even a C++ test.

I.e. if I can remove the -Wno-xxx flag and no warnings show up then its not doing anything right?

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, I guess so. We should just remove the comment, too, then.

Comment on lines -6908 to -6909
# remove -g, so we have one test without it by default
self.cflags = [x for x in self.cflags if x != '-g']
Copy link
Member

Choose a reason for hiding this comment

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

Are we worried about losing this coverage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have versions of the core test suite that include -g and tests that don't.

Notably I don't think any of the ones that we run by default include -g .. so this just seems pointless.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

lgtm % open comments

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM with comment removed.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 5, 2025

LGTM with comment removed.

oops, i thought I had done that already.

- Avoid assignment (prefer mutation) to self.cflags (see emscripten-core#24860 for why
  this is better).
- Parameterize test_wasm_intrinsics_simd and remove unneeded arguments.
- Use inline cflags where it improves test readability.
- Remove use of EMSCRIPTEN_KEEPALIVE with `main` function.
@sbc100 sbc100 enabled auto-merge (squash) August 5, 2025 18:48
@sbc100 sbc100 merged commit 01628ae into emscripten-core:main Aug 5, 2025
29 of 30 checks passed
@sbc100 sbc100 deleted the cleanup_tests branch August 5, 2025 19:58
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