Skip to content

Unify use of std::shared_ptr in module constructor

Stephan Lachnit requested to merge slachnit/allpix-squared:p-shared_ptr-ref into master

See discussion at !756 (comment 6183048)

This change the behavior of
copying a shared_ptr in the function call and them moving that object to a member variable
to
passing a shared_ptr as const reference and the copying it when assigning a member variable.

Performance should be similar, but I find the later approach more readable. This MR also makes this consistent across all modules, since some already used const std::shared_ptr<Detector>& - including changing the Module class. Thus only considerable for v3.

I also found that make_module.sh does not handle shared_ptrs properly, it doesn't provide them as const& nor does std::move - also fixed.

I want to point a certain piece of code out I found often in module constructor:

BlaModule::BlaModule(Configuration& config, Messenger*, std::shared_ptr<Detector> detector)
    : Module(config, detector), detector_(std::move(detector))

While not incorrect, I find it dissatisfying to use an object in the same line in which you move it. Doing this in the opposite order is a bug (which we don't do obviously, but with a const& this is not possible).

In terms of user writing modules, I think the const& approach is more understandable (and easier to use) than the copy-by-value-then-std::move approach.

Would do this before !900 (merged).

Merge request reports