plumed / plumed2

Development version of plumed 2
https://www.plumed.org
GNU Lesser General Public License v3.0
357 stars 283 forks source link

Format of the manual #1046

Closed GiovanniBussi closed 5 months ago

GiovanniBussi commented 6 months ago

There's something wrong with the format of the manual for the master branch.

It looks like verbatim regions associated to plumed input files are not properly closed:

image

GiovanniBussi commented 5 months ago

@gtribello I think this is also something that you probably know how to address. Thanks!

GiovanniBussi commented 5 months ago

I have some guess on this. Currently, this leads to a segfault:

plumed gen_example --plumed plumed.dat --status working --out ooo.html

with

d: DISTANCE ATOMS=1,2
PRINT ARG=d

The segfault happens in GenExample::printExampleInput, at the calls to castToActionWithValue() and castToActionWithVirtualAtom().

I guess that the myplumed.getActionSet().selectWithLabel<Action*>(lab) pointer is null, which can be used as an argument to dynamic_cast but not to launch the virtual function. I will check this tomorrow.

gtribello commented 5 months ago

Hi @GiovanniBussi

I was looking at this at the same time and coming to a similar conclusion. If you change line 312 to:

ActionWithValue* av=myplumed.getActionSet().selectWithLabel<ActionWithValue*>(lab);

and line 336 to:

ActionWithVirtualAtom* avv=myplumed.getActionSet().selectWithLabel<ActionWithVirtualAtom*>(lab);

Then the segfault disappears. I also tried building the manual with this change and it looks much better.

Is there a reason we have to use castToActionWithValue here? I don't see why there would be. I know the select thing is slow but it is not code that needs to be optimised so who cares surely. I think if you make the changes above this reverts this command line tool back to the way it worked in v2.9.

GiovanniBussi commented 5 months ago

I think that: in v2.9 there is a select followed by a dynamic_cast. In master I was changing all dynamic cast to virtual functions for efficiency. Which is useless here because efficiency is irrelevant and because select is already doing a lot of dynamic casts per se. Plus it introduced this subtle bug....

I think the code that you propose is more clear than the original one and not buggy

This raises the fact that I should check all my replacements of dynamic cast with virtual functions because for a null pointer the result is different