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

[pleaese review] c++17: dynamic exception specification -> noexcept(false) #13

Merged
merged 8 commits into from
Mar 21, 2019

Conversation

looooo
Copy link
Contributor

@looooo looooo commented Feb 5, 2019

for conda we need to build smesh with c++17 because boost is also build with c++17.
conda-forge/boost-cpp-feedstock#43
conda/conda-build#3375

c++17 does not allow dynamic exception specifications. This PR replaces these with noexcept(false).

@looooo looooo mentioned this pull request Feb 5, 2019
@looooo looooo changed the title c++17: dynamic exception specification -> noexcept(false) [wip] c++17: dynamic exception specification -> noexcept(false) Feb 10, 2019
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@looooo looooo changed the title [wip] c++17: dynamic exception specification -> noexcept(false) [pleaese review] c++17: dynamic exception specification -> noexcept(false) Mar 6, 2019
@trelau
Copy link
Owner

trelau commented Mar 8, 2019

@looooo A few things (as I haven't had much time to look at any of my now open source hobby projects):

  • It seems the current build is failing due to some missing tbb libraries? I'd like to resolve that first and get master back to building along w/ any PRs.
  • When you replace with noexcept, what happens to the comments that might be thrown in the Salome Exceptions like here? I'm by no means a C++ expert so it's just not clear to me what the impact of this change is.
  • Would it be possible to adjust the appveyor build process (once the current one is fixed) to extend to c++17?

@looooo
Copy link
Contributor Author

looooo commented Mar 8, 2019

It seems the current build is failing due to some missing tbb libraries? I'd like to resolve that first and get master back to building along w/ any PR

Would it be possible to adjust the appveyor build process (once the current one is fixed) to extend to c++17?

I am using this branch for the conda-build and have passing builds for all supported platforms. https://github.com/conda-forge/smesh-feedstock/commits/master The failing builds are builds done on azure which is still in a testing phase.
We could simple use this updated feedstock as base for the appveyor-build. I guess once the azure builds work, we can switch over to this service and test all platforms.

When you replace with noexcept, what happens to the comments that might be thrown in the Salome Exceptions like here? I'm by no means a C++ expert so it's just not clear to me what the impact of this change is.

This part I also didn't understand fully. But from discussions I get the impression that this is an obvious change.

@trelau
Copy link
Owner

trelau commented Mar 9, 2019

This part I also didn't understand fully. But from discussions I get the impression that this is an obvious change.

Seems like we might want to add a unit test for at least one or two of these errors. Make sure it throws an exception in both c++14 and c++17. Or something...

I would prefer to have CI builds in this repo to check PR's and such instead of relying on an outside feedstock if that's what you are suggesting.

@looooo
Copy link
Contributor Author

looooo commented Mar 21, 2019

Ci-check works again. Would be nice if this can be merged.

@trelau trelau merged commit 7168ecb into trelau:master Mar 21, 2019
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.

2 participants