generated from NOAA-OWP/owp-open-source-project-template
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Bmi_Adapter cleanup, de-template #646
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PhilMiller
force-pushed
the
bmi-adapter-cleanup
branch
from
September 23, 2023 02:51
c86efa7
to
d9a2004
Compare
PhilMiller
force-pushed
the
bmi-adapter-cleanup
branch
from
October 13, 2023 18:25
d9a2004
to
936c7c0
Compare
PhilMiller
force-pushed
the
bmi-adapter-cleanup
branch
from
November 7, 2023 23:35
936c7c0
to
8707e5c
Compare
…ince their handling of lifecycle is disparate
PhilMiller
force-pushed
the
bmi-adapter-cleanup
branch
from
November 8, 2023 01:46
b1d3014
to
32824d6
Compare
This avoids giving the impression that there should ever actually be shared ownership - a model instance is always exclusively held by the single adapter instance that created it. Leave the C++ adapter alone for now due to the more complicated lifetime logic that it implements. Leave the Python adapted alone because the unit tests do silly things with reaching in and grabbing the underlying object.
PhilMiller
force-pushed
the
bmi-adapter-cleanup
branch
from
November 8, 2023 22:17
e1d2bef
to
1e0ab7d
Compare
I think this is ready for re-review and integration. I'm done poking at the adapters for now. I'm going to try to tackle some of the rampant silly duplication in the Formulation classes next. |
program--
approved these changes
Nov 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Templates that don't actually provide any polymorphic behavior are just a waste of compilation time and binary size. So, get rid of one.
Also, implementation inheritance (the
bmi_model
member) isn't very favorable in general, and even less so when the base class isn't actually providing anythingChanges
Bmi_Adapter
class to make further refactoring easiershared_ptr<T> bmi_model
member down to child classesBmi_Adapter
= delete;
bmi_model
' to various backing types fromshared_ptr
tounique_ptr
to clarify that they have strict exclusive ownershipTesting
Ordinary CI test suite
Checklist