Skip to content
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

1637 dynamic keywords #1766

Merged
merged 14 commits into from
Jan 18, 2024
Merged

1637 dynamic keywords #1766

merged 14 commits into from
Jan 18, 2024

Conversation

rprospero
Copy link
Contributor

The primary change here is that every keyword is now contained within a specific KeywordStore. There is no longer a static singleton vector in either KeywordBase or in KeywordStore. This leads to a couple of other changes.

  1. Since there is no static singleton, removing references to an object in CoreData involves removing the references in every module of every layer. This is handled in the objectNoLongerValid function in coreData.cpp. This is now a function in the module and not a private function in the class because it needs access to the Module and ModuleLayer classes in order to be built. This wouldn't be an issue, but it's a template function, so leaving it in the .h file creates an import loop.
  2. The Module and ModuleLayer classes now need mutable access to CoreData, since they might be removing references from it (an obviously mutable action). This has resulted in the change from OptionalReferenceWrapper<const CoreData> to Core Data*. I did use a const_cast in layerFuncs.cpp rather than update the entire call chain to provide a mutable CoreData reference, but I'm willing to change this if other people find it upsetting.

This closes #1637

@rprospero rprospero added Scope: Codebase Related to source code management, build etc. 4 DIfficulty: 4 Priority: Medium Indicates an Epic with medium priority Scope: Design Related to fundamental code design, algorithm use etc. labels Jan 17, 2024
Copy link
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confess I'm a little confused why the const_cast and the conversion to a pointer is necessary since, as far as I can see, the CoreData reference was passed non-const down the chain anyway. Indeed I just tried to revert that change locally (i.e. go back to OptionalReferenceWrapper<CoreData>) for both models and all seems to work as expected...

@@ -254,4 +254,5 @@ class CoreData
void removeReferencesTo(Configuration *data);
void removeReferencesTo(Species *data);
void removeReferencesTo(SpeciesSite *data);
void removeReferencesTo(Isotopologue *data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic as I am I would move this up a few lines so as to make the "list" alphabetical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only did I move Isotopologue up a few lines, I also moved Module down a few lines. That leaves gets the whole thing alphabetised.

src/keywords/store.h Outdated Show resolved Hide resolved
Copy link
Contributor

@rhinoella rhinoella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Also agree with what @trisyoungs mentioned regarding the const_cast

@rprospero rprospero merged commit 90e992b into develop Jan 18, 2024
7 checks passed
@rprospero rprospero deleted the 1637-dynamic-keywords branch January 18, 2024 14:11
rprospero added a commit that referenced this pull request Mar 11, 2024
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 DIfficulty: 4 Priority: Medium Indicates an Epic with medium priority Scope: Codebase Related to source code management, build etc. Scope: Design Related to fundamental code design, algorithm use etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Static Singletons
3 participants