Necessary Maintenance #24524
Clang-tidy issue cert-err60-cpp
A minor issue, but it would be nice to have cet::exception conform to this requirement...
/home/eflumerf/Desktop/artdaq-mrb-base/srcs/artdaq/artdaq/DAQrate/SharedMemoryEventManager.cc:96:9: warning: thrown exception type is not nothrow copy constructible [cert-err60-cpp] throw cet::exception(app_name + "_SharedMemoryEventManager") << "Unable to attach to Shared Memory!";
#1 Updated by Kyle Knoepfel 8 months ago
- Status changed from New to Feedback
Eric, thank you for bringing this to our attention. This issue requires some teasing out, and the following links are relevant to this discussion:
Reference 1 discusses the dangers of allowing an exception to escape the exception type's copy constructor. Indeed, if the copy constructor throws an exception that is not handled within the constructor, an abrupt program termination occurs. Reference 1 also presents some solutions to make exception types compliant wrt ERR60, each solution entailing changing the implementation of the exception class.
However, there is at least one more solution possible, which is referred to in reference 2. As the C++ core guideline states, functions should be labeled
noexcept when an escaped exception is either impossible or unacceptable. If an exception were to be thrown during copy-construction of a cet::exception object, it would likely be due to memory exhaustion. Under such a circumstance, program recovery is incredibly unlikely. Fortunately, in roughly a decade of art support we are aware of no circumstance ever occurring.
Rewriting the class to avoid program termination on copy construction would be quite awkward. However, we think adding the
noexcept specifier to the copy constructor is reasonable. This would:
- State the intention that no exception escaping the copy constructor is allowed, resulting in program termination if an attempt is made.
Please let us know if you wish to discuss this further.
#2 Updated by Eric Flumerfelt 8 months ago
This looks good to me, and I definitely agree that any major API changes at this point would be much more damaging than the as-yet-unseen unexpected termination that the error is warning about. I've been using
// NOLINT(cert-err60-cpp) to silence this issue, but if you're willing to add a noexcept, that's much better ;).