Skip to content

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

Merged
merged 3 commits into from
Jun 10, 2022

Conversation

brendandahl
Copy link
Collaborator

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.

Copy link
Member

@kripken kripken left a 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?

@kripken
Copy link
Member

kripken commented Jun 2, 2022

Code size failures on CI might be fixed by merging latest main.

@brendandahl
Copy link
Collaborator Author

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 If this is expected, rerun the test with --rebaseline to update the expected sizes?

@kripken
Copy link
Member

kripken commented Jun 3, 2022

Oh, I think the code size change is unrelated. We just landed a PR to fix it now on main, so merging that here should fix it.

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 --rebaseline). But meanwhile, all PRs will show such errors. Usually that's fairly rare, at least...

Hunter Richards and others added 3 commits June 9, 2022 21:19
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.
@david-fong
Copy link
Contributor

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?

@kripken
Copy link
Member

kripken commented Jun 23, 2022

@david-fong I think it's worth mentioning, good point. A PR for that would be appreciated!

prideout added a commit to google/filament that referenced this pull request Jul 15, 2022
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.
prideout added a commit to google/filament that referenced this pull request Jul 27, 2022
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.
prideout added a commit to google/filament that referenced this pull request Jul 27, 2022
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.
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