openigtlink / OpenIGTLink

Free, open-source network communication library for image-guided therapy
http://openigtlink.org
BSD 3-Clause "New" or "Revised" License
102 stars 184 forks source link

FIX: make code compile under C++20 #257

Closed texel-sensei closed 2 years ago

texel-sensei commented 2 years ago

In the destructor of the igtlLightObject, a call to std::uncaught_exception was used. This function was deprecated in C++17 and then removed in C++20, which causes OpenIGTLink to fail compilation on Visual Studio if C++20 is enabled. Judging from the comments in the code, the call was unnecessary, so it was removed.

leochan2009 commented 2 years ago

Hi, though the chance is low, the uncaught exception might still pose some value here. As well as for the backward compatibility concern, could we follow the link here? https://github.com/doctest/doctest/pull/340/commits/e61a0708b205459e478dfaa94dcb82f5b015334a

texel-sensei commented 2 years ago

The code guarded by the if does not throw an exception, as that line was commented out and replaced by a warning.

And no matter the guard, if an exception would actually be thrown at this part, the program would exit via std::terminate. Since C++11 all destructors are implicitly defined as noexcept, so throwing an exception here would be a throw from a noexcept(true) function, which results in terminating the program.

Edit: To elaborate further, I did actually have a version that used the feature test macros to conditionally switch between std::uncaught_exception and std::uncaught_exceptions in the beginning. But I decided against that solution, as it introduces a lot more code, for what is at the end the printout of a single warning. I would argue that the behaviour without the uncaught exceptions check is even better than previously. The warning is printed in a case where basically everything is lost. So it's better to always see this warning, to aid in debugging instead of surpressing it when there is currently an exception propagating.

leochan2009 commented 2 years ago

@texel-sensei , it seems good to me. @tokjun , if there is no further concern, i will merge this pull request.

tokjun commented 2 years ago

@leochan2009 yes it sounds good to me. Thank you, @texel-sensei @leochan2009