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

Preprocessor: Use eval() for good #7651

Merged
merged 10 commits into from
Dec 13, 2018
Merged

Preprocessor: Use eval() for good #7651

merged 10 commits into from
Dec 13, 2018

Conversation

kripken
Copy link
Member

@kripken kripken commented Dec 12, 2018

This replaces our custom preprocessor code with eval(). This allows arbitrary conditions in our #ifs, e.g. the __EMSCRIPTEN_HAS_nodefs__ things are now

#if LibraryManager.has('library_workerfs.js')

where that function is a new utility that checks if a library was included.

This patch also switches to things we've wanted to do, like

#if FULL_ES2 || LEGACY_GL_EMULATION

where in the past we had to create a new global for them.

We now error if we do if we ifdef on something nonexistent (the eval call throws), which noticed that we had a bunch of NO_DYNAMIC_EXECUTION code that could be removed (we removed that option a while ago).

Copy link
Contributor

@jgravelle-google jgravelle-google left a comment

Choose a reason for hiding this comment

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

;_; but yeah this is so much better

@@ -603,15 +603,15 @@ Module['registerFunctions'] = registerFunctions;
#endif // RELOCATABLE
#endif // EMULATED_FUNCTION_POINTERS

#if WASM_BACKEND_WITH_RESERVED_FUNCTION_POINTERS
#if WASM_BACKEND && RESERVED_FUNCTION_POINTERS
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also yank out the WASM_BACKEND_WITH_RESERVED_FUNCTION_POINTERS definition from jsifier.js, because this should be all that uses it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed

@kripken
Copy link
Member Author

kripken commented Dec 12, 2018

I undid the embind changes regarding NO_DYNAMIC_EXECUTION - I realized those were wrong. We will need a different solution there, I think there's a bug in the usage of that flag.

@kripken
Copy link
Member Author

kripken commented Dec 12, 2018

Turns out #7653 will fix the NO_DYNAMIC_EXECUTION stuff which was an unrelated embind bug. So we just need to wait on that.

@kripken
Copy link
Member Author

kripken commented Dec 12, 2018

Actually, this blocks #7612, so let's land it with a small workaround for that embind issue, that we can remove soon.

@wffurr
Copy link
Contributor

wffurr commented Dec 12, 2018

Thanks for this - I was considering making #if on a non-existent setting an error as well. Happy to see you have already done so.

@kripken kripken merged commit 5247935 into incoming Dec 13, 2018
@kripken kripken deleted the preproc branch December 13, 2018 01:03
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.

None yet

3 participants