-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add C preprocessor support to JS libraries. #1969
Conversation
…use of #if <expression with (), &&, ||, <, >, <=, >=, ==, !=, defined()>. Adds support for #define, #undef, #ifdef, #ifndef, #elif, #warning and #error. Does not (yet?) implement support for preprocessor macros, or text substitution outside preprocessor directives with #define. Add unit test.
There is a bug that previously committed optimizations silently failed due to C preprocessor not working as expected. Lines like https://github.com/kripken/emscripten/blob/master/src/library_gl.js#L4195 were not properly evaluated, which caused the optimizations to fail. |
Curious about the motivation here? Just for future use, or something concrete? |
In addition to fixing the abovementioned case where things silently failed with
and duplicate the same section of code. I figured you wouldn't let such a PR pass, so decided to first implement better preprocessor support to be able to do that. |
@@ -14,53 +14,369 @@ function processMacros(text) { | |||
}); | |||
} | |||
|
|||
// O(len(start)) method for checking if str begins with start. | |||
function strStartsWith(str, start) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use a regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regexes aren't that readable or self-descriptive. I think this is more documentative, plus probably faster as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am 99% sure this would be much slower, regexes are JITed extremely efficiently in modern JS engines.
My main concern is that this is a big new chunk of code that makes the core compiler more complex, and we would need to maintain in the future. So I am strongly inclined to specifically fix the issues you mention of |
I thought about eval(), but then rejected it as a deadend since I don't think it will be easily possible to make it support preprocessor expansion, macros, ..., VA_ARGS or # or ## operators in the future if needed. I also searched if there would be an existing C preprocessing library out there implemented in JS, but could not find anything suitable. People expect a lot of things from their existing knowledge when they see C preprocessing directives. The current code hardcodes the few cases it supports, and silently fails on the ones it doesn't. I don't think it's mature for the project to take the stance that only those preprocessor directives that our src/library_xx.js happen to use are supported, and incrementally keep hardcoding new cases whenever people report bugs against them (those recent #if 1 and #include x.js for example). I wanted to find the proper abstractions here that would have at least some chance of being future-proof, while trying to keep the code in a manageable size. Another route could be to reuse Clang preprocessor with Clang -E option, which would be a full black box process. |
I wonder if this could be useful: https://github.com/mozilla/pdf.js/blob/master/external/builder/builder.js#L23 I don't believe it could support ## though, but maybe the flexibility makes more sense for JS |
That looks like nice code. But with a little refactoring I think ours could do everything they do. @juj, my concern with your code is the size and complexity, in a part of the compiler I really want to keep as simple as possible. I agree we should not fail silently on things we don't support, once we do that, I think we would be in pretty good shape, and if people ask for more features, we could add them like |
Implement better support for C preprocessing directives. Enables the use of #if <expression with (), &&, ||, <, >, <=, >=, ==, !=, defined()>. Adds support for #define, #undef, #ifdef, #ifndef, #elif, #warning and #error. Does not (yet?) implement support for preprocessor macros, or text substitution outside preprocessor directives with #define. Add unit test.