-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add support for noexcept keyword to embind. #17140
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
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 don't fully understand the C++ parts, I admit 😄 but it looks reasonable. Testing looks good.
@sbc100 maybe you also want to take a look?
Code size failures on CI might be fixed by merging latest |
I'm not really sure why the code size would change. I'm guessing that's pretty common when changing embind.h though. Should I do the suggested |
Oh, I think the code size change is unrelated. We just landed a PR to fix it now on Sorry for the annoyance. The issue is that we don't run code size tests on the chromium rollers, because if we did then any LLVM change that changes expectations would break the rolls, which then needs to be fixed in a multi-step process. Instead, we skip code size tests there, and then we can see the effects here on github, where the fix is pretty simple (land a change after doing |
This continues the work from emscripten-core#15273 and emscripten-core#10613 which supports the noexcept keyword by ignoring it on c++17 compilers. I've also added a missing template for class methods and tests for each possible use of noexcept.
Thanks everyone for working on this. I'm quite happy that this got merged! Just curious about why this wasn't included in the changelog entry for 3.1.14. Was it considered too small a fish to mention? |
@david-fong I think it's worth mentioning, good point. A PR for that would be appreciated! |
Starting with 3.1.14, embind started to support for `noexcept` which caused multiple definition errors since we have a workaround in place that alreadys supplies template instantiations for `noexcept`. This change should not affect G3 since our JS bindings are not used in G3. The upstream fix is here: emscripten-core/emscripten#17140 Fixes #5789.
Starting with 3.1.14, embind started to support for `noexcept` which caused multiple definition errors since we have a workaround in place that alreadys supplies template instantiations for `noexcept`. This change should not affect G3 since our JS bindings are not used in G3. The upstream fix is here: emscripten-core/emscripten#17140 Fixes #5789.
Starting with 3.1.14, embind started to support for `noexcept` which caused multiple definition errors since we have a workaround in place that alreadys supplies template instantiations for `noexcept`. This change should not affect G3 since our JS bindings are not used in G3. The upstream fix is here: emscripten-core/emscripten#17140 Fixes #5789.
This continues the work from #15273 and #10613 which supports the noexcept
keyword by ignoring it on c++17 compilers. I've also added a missing
template for class methods and tests for each possible use of noexcept.