Refactor: replace static colvarmodule with class member

[jhenin]

In a first stage, just trying to keep functionality intact, removing all static member data from class colvarmodule.

First approach: make a colvarmodule pointer a member of colvarparse, because the big classes inherit from it.

Now a number of lightweight classes (in colvartypes) or sub-classes don't have access to the pointer yet. Alternatives are to extend them with a pointer, or remove the functionality that needs it (mostly log and error calls).

EDIT: because this is so far-reaching, I am turning it into a partial implementation that can be merged in finite time, leaving some of the classes to be modified later.

Namely:

• colvargrids_ - make broad use of cvm functions, passing pointers to the module between them is very heavy
• colvarproxy_* base classes. They all implement partial functionality for colvarproxy. I am not sure anymore that inheritance is the best paradigm, or we would need a common base class colvarproxy_base at the root of a diamond inheritance.

colvarbias classes have been handles thanks to virtual inheritance: only the colvarbias base class constructor sets the cvmodule pointer, as passed on by the final classes. Intermediate classes don't have to worry about it.

Final status: the remaining uses of the static pointer can be tracked by looking for calls to cvm::main(). There are new functions log_static and error_static that make use of the static pointer only if available, and have fallbacks if not. All of these can be phased out in later PRs.

[jhenin]

A lot of progress towards the goal. Some classes got access to a colvarmodule pointer. Others got access to a static version of the error that is fatal - mostly assertions detecting bug conditions.

The main hurdle that remains is the scripts, which rely heavily on the static colvarscript_obj() to access the C++ object from C functions. In practice, one script interpreter should interact with only one colvarmodule object through one colvarscript object. I'm not sure how to expose the correct pointer to each interpreter if there are two.

As a first step, we could keep a static, global colvarscript pointer. Admittedly that pushes a part of the problem further down the road, but at least it would let us design everything to work well without static colvamodule pointer from now on.

[giacomofiorin]

This is great progress already! :-)

For Tcl, we could maybe use Tcl_SetAssocData() and Tcl_GetAssocData()? VMD uses this to get a pointer to the main object from the interpreter.

That would only work for Tcl, but that's also where you would need it. LAMMPS scripting commands are associated to a Fix object, so no gymnastics needed

[giacomofiorin]

My main concern is, all Colvars functionality relies on there being a single instance of the module. Removing static pointers would also require adding a different kind of check that there is only one colvarmodule object.

[HanatoK]

My main concern is, all Colvars functionality relies on there being a single instance of the module. Removing static pointers would also require adding a different kind of check that there is only one colvarmodule object.

Why does all functionality rely on being a single instance?

[giacomofiorin]

Why does all functionality rely on being a single instance?

Mostly for I/O: if multiple instances were allowed, each would need its own set of input and output files.

[HanatoK]

Mostly for I/O: if multiple instances were allowed, each would need its own set of input and output files.

But this is what the derived classes of colvarproxy should take care.

[jhenin]

In practice, the main application I've had in mind so far would be associating one colvarmodule instance with one molecule in VMD, although that would need more work because 1) VMD has a single Tcl interpreter, and 2) so far we have single proxy object - so we'd have to choose between duplicating the proxy object as well, or let a single proxy switch between molecules. Given the current architecture, I think one proxy per molecule would be simpler.

Thanks to Giacomo's suggestion with Tcl_SetAssocData, I think this is workable: the interpreter could register not just one pointer, but say, one proxy pointer for each loaded molecule. If such a pointer were missing, the script interface would know to allocate a new instance for that molecule.

You may have more ideas for using multiple instances: I'm interested.

[jhenin]

To answer @giacomofiorin 's remark about relying on there being a single instance: my goal here is that several instances could exist and that all calls are addressed to the right objects. One point to be worked out is whether they attach to the same or different proxy objects. For now, I am working under the assumption of a proxy object talking to a single colvarmodule instance.

[jhenin]

Making these changes, I find that the scripting functionality would be better located in the colvarmodule class than colvarproxy. The colvarscript object is now created and used only by the module, and the proxy is just an extra level of indirection.

[jhenin]

I had to re-introduce a patch to ScriptTcl.C to pass the correct colvarscript pointer in the NAMD-defined version of the cv command. Incompatible (non-patched) NAMD will pass a wrong pointer, so I added a validation function to colvarscript to detect that and print a helpful error message.

[jhenin]

I think the last issue in Gromacs is fixed. Note that at present, in shared-memory settings, only one thread has a pointer to the colvarmodule object, but in principle, it could be broadcast or made static so all threads can access it if that was useful.

[jhenin]

Since this is very conflict-prone, I propose to rebase only when ready to merge.

[giacomofiorin]

I haven't looked at the entire diff, which is large, but everything I have seen looks okay. Basically, every class now needs an extra parameter, but this is a necessary evil to remove the static colvarmodule pointer.

Another necessary evil are the "*_static" functions, which would have to be removed in the next iteration.

[jhenin]

Another necessary evil are the "*_static" functions, which would have to be removed in the next iteration.

Agreed! I realized that completing this job would be too far-reaching for a single PR, as several class hierarchies would have had to be reworked. For the classes that I didn't want to overhaul right now, I added these functions that behave just like in the current master branch, but are clearly named as using the deprecated coding pattern. This way we can move forward with the rest of development and take the time to finish the transition.