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 remove modules singleton #1749

Merged
merged 7 commits into from Jan 12, 2024
Merged

Conversation

rprospero
Copy link
Contributor

As the first half of #1637, this PR removes the static vector of modules in the modules class and, instead, makes CoreData responsible for tracking the modules being used. This allows multiple simultaneous instances of Dissolve and CoreData that can have completely different sets of modules.

A couple of notes:

  1. Many functions now need to be passed as CoreData term to indicate which set of modules is to be accessed.
  2. Some functions that used to be constant in CoreData now require a mutable version, since the list of modules is modified.

Conundrum

As mentioned above, some functions are being given full mutable access to CoreData, although they only need mutable access to the module list. We could rework most of the Module class static members to merely take the list of modules instead of the full CoreData class, resulting in better const correctness. However, this comes at a cost of encapsulation, since we're making the separation more explicit. I'll leave it to the reviewer whether this is a) necessary, b) worth a separate issue, or c) completely unnecessary

@rprospero rprospero added Scope: Codebase Related to source code management, build etc. 16 DIfficulty: 16 labels Dec 12, 2023
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.

One singleton down. I don't particularly mind the need to pass non-const CoreData around - after all it is the guts of the entire simulation, so protecting it is always going to be hard!

With regards to the Module conundrum, I would suggest that we could do away with the static functions in the Module class itself and move them to CoreData. That seems the neatest solution now that the module class is not self-managing an instances vector.

Since the Module class no longer statically tracks the module
instances, these functions made more sense as members of CoreData
@rprospero
Copy link
Contributor Author

@trisyoungs As per your comment, I've eliminated the static methods in Module and moved them into CoreData as const methods.

@trisyoungs trisyoungs added this to the Release 1.5 milestone Jan 12, 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.

Looking good. I do have one of my annoying naming comments, however! Some functions have a member variable mutCoreData_ with the name reflecting the fact that the CoreData is mutable. We don't do this anywhere else in the code, and it feels like we don't want to set a precedent here for "nominative determinism of mutability" in the codebase, so I would have to suggest reverting them to just coreData_.

Yes, there's another `coreData_` in the parent class.  Don't think
about it.
@rprospero
Copy link
Contributor Author

rprospero commented Jan 12, 2024

@trisyoungs The mutCoreData_ member is a bit more pernicious than you may have realised. Specifically, it appears in a couple of subclasses of KeywordWidgetBase, which happens to already have a coreData_ member that has been declared const. Since the name coreData_ was already taken, but was const, I just created the second reference to cover the issue.

Granted, this was my fault for not understanding C++. I assumed that someone would stop me from redefining the member from dropping the const contraint of the parent class, but the compiler didn't have a problem with it. Oddly enough, I do get issues about intialisation that seem to indicate that the class now has two members with the same name, but they seem to be working correctly, so I won't complain.

@trisyoungs
Copy link
Member

@rprospero Ah, I see, although I am surprised that the compiler (standard?) lets you change const-ness in the derived class.

@rprospero rprospero merged commit 4dbdad4 into develop Jan 12, 2024
7 checks passed
@rprospero rprospero deleted the 1637_remove_static_singletons branch January 12, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
16 DIfficulty: 16 Scope: Codebase Related to source code management, build etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants