search

trelau / SMESH

Mesh module from the Salome Platform
GNU Lesser General Public License v2.1
52 stars 32 forks source link

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

Closed looooo closed 5 years ago

looooo commented 5 years ago

for conda we need to build smesh with c++17 because boost is also build with c++17. https://github.com/conda-forge/boost-cpp-feedstock/issues/43 https://github.com/conda/conda-build/issues/3375

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

trelau commented 5 years ago

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

looooo commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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