-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
This compiles, but does not work
There was a problem hiding this 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
@trisyoungs As per your comment, I've eliminated the static methods in Module and moved them into CoreData as const methods. |
There was a problem hiding this 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.
@trisyoungs The 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. |
@rprospero Ah, I see, although I am surprised that the compiler (standard?) lets you change const-ness in the derived class. |
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:
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