Refactor FieldData to be base class of Field
#2313
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
The Coordinates' metric elements can hold shared_ptrs that result in a cyclic dependency between the Fields and the Coordinates, with the result that some unit tests have memory leaks. One place the issue can arise is when multiplying fields by metric elements with `CHECK > 0`. Then, `ASSERT1_FIELDS_COMPATIBLE` checks that both fields share a `Coordinates` -- but this has the side effect of initialising the `shared_ptr` in the metric elements. This means that when the `Mesh` gets destroyed, the `coords_map` starts to destroy its contents, but because the `Coordinates` metric elements members have references to the `Coordinates`, the `Coordinates` in `coords_map` doesn't get destroyed. The end result is that the `Coordinates` gets leaked. The fix here is to use a `weak_ptr` in `Field`. This only gets a reference to the `Coordinates` when needed.
This removes the double-inheritance from `Field2D` and `Field3D`, as well as making `FieldPerp` a subclass of `FieldData`
* fix-field-coords-leak: Fix memory leak of Coordinates in unit tests
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Boundary operators should only be copied in copy construction
And braces around if-statement body
|
@bendudson I just fixed the few clang-tidy warnings, please could you re-approve? |
bendudson
approved these changes
May 13, 2021
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.
The reason for doing all of this was originally to be able to remove uses of
bout::global::meshfromFieldData, so that I could add the fix for the case-sensitiveOptionsfor ignoring the boundary region inputs without adding more uses of the global mesh. There were a few other plus points to doing this, namely removing the double-inheritance fromField2/3D, and tidying up the code a bit. It has turned out to be a bit of a bigger effort than I had anticipated/hoped for!Make
FieldDatathe base class ofField, and move some members forFieldintoFieldData. This does a few things:Field2DandField3DFieldPerpnow also ultimately inherits fromFieldData, where it previously was onlyFieldMesh,Coordinates, andCELL_LOCmembers fromFieldtoFieldDataVector2/3Dnow havegetMesh()andgetCoordinates()methods, which means we don't need to do the hackyv.x.getMesh()etc.Field::copyFieldMemberswith more idiomaticField::operator=(rhs)calls in the derived classes, and makes sure the copy-constructor methods delegate to theFieldconstructorFieldDatamembersprivaterather thanprotectedprotectedmembers are bad now! They make refactoring that much more awkwardfieldmeshis stillprotectedbecause it's used in a lot of places. I should probably also bite the bullet and make thisprivatetooFieldData, and the unused experimentalFieldVisitorbout::globals::meshfromFieldDataAlso includes #2306 so that I didn't have to deal with the conflicts separately.
If we were to go further here, I think we could do some or all of the following:
FieldData::applyBoundary(bool)have a default implementation -- it appears to be basically the same between all the implementationsFieldtoScalarFieldand add aVectorFieldbase class to remove duplicated code from the vector classesderiv) into one of the base-classes and make it aunique_ptr-- or even just a value?