-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Don't include internal compiler settings with RETAIN_COMPILER_SETTINGS #25667
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
Conversation
5a2713b to
1f2c4bc
Compare
|
I'll defer to @kripken on this - I do not recall what the context was around adding this feature, and what the usage were. Might be worth mentioning specific examples in the ChangeLog of which settings in particular will no longer be accessible, that people might have been referring to. |
|
OK, will wait for @kripken to chime in |
The fact that the test code had |
I don't actually know if this feature has any users at all. It was adding way back in 2014 but without much rational: 153f2d9 I can't find any users with google. There are some users on github .. I'll investigate more. |
|
It looks like there are 14 users on github: https://github.com/search?q=emscripten_get_compiler_setting%28%22++NOT+path%3Aemscripten.h++NOT+path%3Aemscripten_get_compiler_setting.c&type=code The settings used are Of these only |
|
There are only two uses of So I think we should go ahead with this change. The other usages are using non-internal settings. |
1f2c4bc to
164083e
Compare
|
OK, I made a carve out for EMSCRIPTEN_VERSION here... so there should be minimal risk of breakages. |
164083e to
7d1df72
Compare
This is technically a breaking change so I mentioned it in the ChangeLog. Fixes: emscripten-core#25663
7d1df72 to
da40019
Compare
This is technically a breaking change so I mentioned it in the ChangeLog.
Fixes: #25663