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.

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

Terminate unterminated statements + fix some type coercion #9019

Merged
merged 11 commits into from
Jul 22, 2019

Conversation

VirtualTim
Copy link
Collaborator

I got sick of seeing so many warnings in the generated code, so I thought I'd fix some of the trivial ones.
This may have a very minor positive impact on performance as it should help the optimisers out.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Where do you see such warnings? What tool produces them? Presumably we can add tests/checks for these to enforce these rules?

@VirtualTim
Copy link
Collaborator Author

VirtualTim commented Jul 19, 2019

They show up in the IDE I'm using - WebStorm.
It actually reports hundreds of warnings, but most of them are due to var usage (lots of duplicate declarations), type coercion (caused by ==), and unused functions/variables.

I'm sure a lot of these are false positives, a function may be unused in the main thread, but used in a worker, but there's at least a few concerning things there. Seeing if (something == 0) makes me a bit uncomfortable, because that's also true if something isn't defined.

I'm sure there's some actual issues there, but I figured I'd just try and fix some of the easy/obvious ones to reduce the noise.

I don't really think this is worth automating, there's just too many false positives at the moment. I guess we could just test a few things, like missing/unnecessary semicolons, but I'm not sure if it's worth the effort.

Edit: It would be nice to restructure things so that there's no undefined variables, because then we could check for that and prevent things like #9025. In my build I'm getting 110 unresolved variables, and I'm sure at least a couple are actual bugs.

@kripken
Copy link
Member

kripken commented Jul 22, 2019

Thanks @VirtualTim! Good to clean this stuff up.

@kripken kripken merged commit 791f8cb into emscripten-core:incoming Jul 22, 2019
@VirtualTim VirtualTim deleted the patch-4 branch July 23, 2019 01:28
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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.

4 participants