refactor: reduce the use of cvm::main()#924
Conversation
| global_cvc_map[def_config_key] = []() { | ||
| return new def_class_name(); | ||
| global_cvc_map[def_config_key] = [&]() { | ||
| return new def_class_name(cvmodule); |
There was a problem hiding this comment.
Would it be possible to avoid this? We can pass the pointer using a base-class method immediately after creating the object
There was a problem hiding this comment.
I am not sure how to avoid it. Could you give more hints?
There was a problem hiding this comment.
I suppose that would mean using a cvc constructor that sets cvmodule to nullptr, and then using deferred initialization. But that seems riskier to me. Right now I'm not sure everything is robust to cvmodule being null.
There was a problem hiding this comment.
Anyway, I have changed the code to avoid capturing the reference of this pointer in d16ae99, since the previous code crashed in VMD.
There was a problem hiding this comment.
I suppose that would mean using a
cvcconstructor that setscvmoduletonullptr, and then using deferred initialization. But that seems riskier to me. Right now I'm not sure everything is robust tocvmodulebeing null.
As hinted in the other comment, yes, I'm thinking about deferring initialization for everything that involves data outside the scope of the object being constructed.
If we were to move all calls to the colvardeps functions to the init() function, what would break?
There was a problem hiding this comment.
I don't think moving the pointer initialization to init() is a good idea. The colvarmodule* pointer should be available at construction, to at least facilitate debugging with cvmodule->log.
There was a problem hiding this comment.
That sounds fine, but for initializing POD members, which is what a constructor should really do, do we need to debug?
There was a problem hiding this comment.
Frankly speaking, I am not sure. colvardeps does not have an init() function, and if I understand correctly what @jhenin was trying to do, the empty constructor colvardeps() is intended to be deprecated, so other classes derived from colvardeps should have colvarmodule* cvmodule_in as a parameter in the constructors.
I have completed most of the refactoring following the same pattern in #886 for derived classes of colvardeps in later commits (see 727952b). The C and fortran interfaces should work as well.
There was a problem hiding this comment.
But we don't need to have an init() function for colvardeps, the one for the class directly derived from it is the one that matters: in this case, colvar::cvc::init().
It's very likely that an Abstract Syntax Tree would require deferred initialization, so I personally would rather work toward that. Am I missing something here?
There was a problem hiding this comment.
Well, I admit that whether using deferred initialization or not is a choice of design. I feel that using colvarmodule* cvmodule_in in the constructors is more robust, and it follows the same pattern in #886 .
The AST should be OK because the tree of colvardeps already uses the pointers:
std::vector<colvardeps *> children;
std::vector<colvardeps *> parents;
|
|
||
| // This constructor depends on a static cvm pointer and is deprecated | ||
| colvar::cvc::cvc() | ||
| : colvardeps(cvm::main()) |
There was a problem hiding this comment.
I think @jhenin may have already thought about improving colvardeps so that it doesn't need this pointer.
There was a problem hiding this comment.
Right now, it's not that colvardeps itself definitely needs the pointer, but rather, colvardeps is used as a vehicle to share the pointer with many classes that inherit from it. If that was to be removed, the pointer would have to be added separately to the cvc, colvar, and colvarbias classes.
There was a problem hiding this comment.
And we'd have the same issue, that all cvc constructors would need the pointer
There was a problem hiding this comment.
The source of the problem is that currently in these constructors we're doing stuff that goes outside the object being constructed. We should rather do that in init(), and that point we would have a valid cvmodule pointer.
| // Utility functions used to query the command database | ||
| extern "C" { | ||
|
|
||
| class colvarscript; |
There was a problem hiding this comment.
I think I need to remove this forward declaration in extern "C" and use an opaque pointer to colvarproxy in the functions below.
There was a problem hiding this comment.
Done. I have changed the C interface functions to use void* proxy_in.
From the C interface, it is impossible to use the colvarscript class or even get a pointer to the colvarscript object. The example C code in C_test.c shows that only an opaque pointer to colvarproxy is available, so the C functions in extern "C" should get the script object from the opaque pointer instead of expecting the script object being passed as a function argument.
|
Superseded by #925. |
3 participants
This PR is intended to partially complete #886.