Detector::panels() returns std::vector<Panel> by value creating dangling references

[dimitrivlachos]

As was noted in: DiamondLightSource/fast-feedback-service@fb3fb44

Detector::panels() returns the panel vector by value:

dx2/include/dx2/detector.hpp

Line 113 in 5b1798c

std::vector<Panel> panels() const;

dx2/dx2/detector.cxx

Line 315 in 5b1798c

std::vector<Panel> Detector::panels() const { return _panels; }

This causes two problems for callers:

1. panels() returns a temporary std::vector<Panel>. Binding a reference to panels()[0] binds to a subobject of that temporary reached via operator[], and C++ temporary lifetime extension does not apply in that case. The vector is destroyed at the end of the statement and panel dangles for its entire remaining lifetime.

   It compiles cleanly with no warnings and fails only at runtime, non-deterministically, since it depends on whether freed heap is reused. In a downstream project this manifested as a corrupt Panel::get_origin() (x/y garbage, varying every process run) and was difficult to diagnose.

2. Even when used safely, every call deep-copies the whole std::vector<Panel>, and each Panel is a heavy struct (multiple Eigenmatrices/vectors).

Fix:

We should refactor this to return a const reference to the member, which is the idiomatic accesor form:

const std::vector<Panel> &panels() const;

const std::vector<Panel> &Detector::panels() const { return _panels; }

And we should audit other accessors within DX2 to confirm that they are adhering to this pattern so that we don't stumble into this same non-error throwing issue.

I created this issue instead of just fixing it as I thought it prudent to document this explicitly.

[jbeilstenedmands]

Thanks for finding this @dimitrivlachos , agreed we should refactor to return const ref.

[dimitrivlachos]

Great, I will go ahead and refactor the accessors then.