Add virtual destructor #12
Conversation
The Bmi base class definition needs to define its destructor as `virtual` to enable derived classes to support accurate destruction. Cf https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dtor This is technically an ABI change impacting any existing code. To avoid potential failures and definite UB from ODR violation, all clients and implementations within a given system will need to be (re-) built against matching versions of this header. It's unclear whether it's a breaking change of the BMI specification itself. The depends on whether the reference to this file is normative or exemplary.
|
The ABI change is probably not too consequential for most users. I'm roughly guessing that the sprawling set of components and large multi-team project of the NWM is going to be the worst impacted. It's also where I saw this come up, due to GCC's |
|
@mdpiper What's the process look like for actually merging a change like this? |
|
@PhilMiller I'd like someone with C++ knowledge to review this. Would you be able to recommend someone? After review, we should be able to merge this with lazy consensus. |
|
@hellkite500 knows the relevant considerations on both the C++ and BMI sides well enough. We'd already discussed the patch before I posted it. Otherwise, I could solicit someone working on the Kokkos Tools, I guess, as folks with ABI evolution constraints on a plugin system design. From a strictly C++ development perspective, I think of this as an utterly mundane, uncontroversial change. The challenge as I see it is entirely from the BMI ecosystem consideration. In the long run, it should fix more bugs than it creates, but it has potential for extended 'short-term' pain as people get the message that they need to transition by rebuilding all their code with the revised header. |
There was a problem hiding this comment.
This change is needed for proper destruction of subclasses of Bmi using dynamic dispatch to virtual functions through a Bmi base pointer. This is common practice in interface design/implementation using C++ .
In practice, I don't think too many implementations of C++ bmi models are going to be using custom destructors, so this probably hasn't been a problem. Typically "destruction" semantics are likely done in the Finalize function and implementers likely don't define custom destruction beyond that in many cases.
However, there may be use cases in which custom destruction semantics are used/needed, and without the virtual destructor, calls to a Bmi base pointer's destructor will only call the (default, do nothing) Bmi::~() and the subclass destructor won't get dispatched to at runtime.
Adding this does, as noted, introduce an ABI change for the C++ implementation only. There doesn't seem to be real consensus yet (see this disscussion thread) as to what level of ABI compatibility BMI should strive to maintain, both at the conceptual interface layer of BMI and at the individual implementations such as bmi-cxx. That said, I think a release tag documenting the change and the possible implications and fix (recompile components with new header) would likely be sufficient to communicate to users/developers.
@RolfHut tagging you here as you have expressed interest in the ABI compatibility, and may want to track this merge and re-build if you have C++ components in any workflows.
|
@PhilMiller I've tagged this |
|
Can we get someone that worked on GRPC4BMI to also review this? As a PI I can not oversee the impact this has, it feels very specific to C++ and not to BMI in general, but this is beyond my knowlegde. Given that in GRPC4BMI we also work with C++, it might impact it. Maybe @nielsdrost can suggest who would be best for this? |
|
@RolfHut @nielsdrost Would you still like someone from @eWaterCycle to look at this PR? If not, let me know & I'll merge it. |
|
yes I would, but personally I can not oversee the scope of this. Maybe @BSchilperoort or @Peter9192, but since they are both not in this org, I will email them. Please give a few days before laze merging. |
|
@RolfHut Since this is such a touchy change, would it be possible to get comments affirming that they think this is OK? As eager as I am to see it move forward, the sort of breakage this can cause if people are caught unaware is really painful to debug, so I'd like to avoid that. |
We haven't had time to look into this in detail, but a dedicated release tag would be great so we can always pin to v2.0 as a (temporary) workaround for unforeseen issues in the future. |
|
@mdpiper see @Peter9192 his comment, if you create that dedicated release tag, than all ok with us from eWaterCycle. |
The Bmi base class definition needs to define its destructor as
virtualto enable derived classes to support accurate destruction.Cf https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dtor
This is technically an ABI change impacting any existing code. To avoid potential failures and definite UB from ODR violation, all clients and implementations within a given system will need to be (re-) built against matching versions of this header.
It's unclear whether it's a breaking change of the BMI specification itself. The depends on whether the reference to this file is normative or exemplary.
Resolves #11