refactor!: move AMVF and IVF config explicitly

[felix-russo]

Title says it all!

Might be breaking as now config must be moved in the parent scope.

It might be better to not move the config and pass it as a const reference, not sure though.

[paulgessinger]

I don't think it has performance implications. The move most likely is going to result in a regular copy anyway, but the compiler might end up optimizing this out the same way it does for the const ref. We also don't (or shouldn't) instantiate this very often (I think we do at this point, but only for convenience reasons) so I don't think it's a critical optimization.

[felix-russo]

Should I just use a const ref then?

[codecov]

Codecov Report

Merging #2509 (8edd312) into main (cc2f639) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2509   +/-   ##
=======================================
  Coverage   49.59%   49.59%           
=======================================
  Files         473      473           
  Lines       26824    26824           
  Branches    12355    12355           
=======================================
  Hits        13304    13304           
  Misses       4753     4753           
  Partials     8767     8767           

Files	Coverage Δ
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.hpp	89.47% <100.00%> (ø)
...clude/Acts/Vertexing/AdaptiveMultiVertexFitter.hpp	73.07% <100.00%> (ø)
...e/include/Acts/Vertexing/IterativeVertexFinder.hpp	80.00% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[github-actions]

📊: Physics performance monitoring for 8edd312

Full contents

physmon summary

• ✅ CKF truth_smeared
• ✅ IVF truth_smeared
• ✅ AMVF truth_smeared
• ✅ Track Summary CKF truth_smeared
• ✅ Seeding truth_estimated
• ✅ CKF truth_estimated
• ✅ IVF truth_estimated
• ✅ AMVF truth_estimated
• ✅ Track Summary CKF truth_estimated
• ✅ Seeding seeded
• ✅ CKF seeded
• ✅ IVF seeded
• ✅ AMVF seeded
• ✅ AMVF (+grid seeder) seeded
• ✅ Track Summary CKF seeded
• ✅ Seeding orthogonal
• ✅ CKF orthogonal
• ✅ IVF orthogonal
• ✅ AMVF orthogonal
• ✅ Track Summary CKF orthogonal
• ✅ Ambisolver seeded
• ✅ Ambisolver orthogonal
• ✅ Seeding ttbar
• ✅ CKF ttbar
• ✅ Ambisolver
• ✅ Track Summary CKF ttbar
• ✅ AMVF ttbar
• ✅ AMVF (+grid seeder) ttbar
• ✅ Truth tracking (GSF)
• ✅ Truth tracking
• ✅ Particles inital fatras
• ✅ Particles final fatras
• ✅ Particles inital geant4
• ✅ Particles final geant4

[acts-project-service]

🔴 Athena integration test results [178a798]

Build job with this PR failed!

Please investigate the build job for the pipeline!

[paulgessinger]

Anyway, implicitly moving mutable references is not a good idea anyway, so let's do this.

[paulgessinger]

This required minor compile fixes in Athena.

[felix-russo]

I realized that we pass config structs almost always as const ref.

Should we do this here too? Or should we also use std::move for the other configs? @paulgessinger

[paulgessinger]

@felix-russo It doesn't really matter. If you pass a const ref and copy into the member in the constructor or you pass a copy and move it into the container. The const ref might be slightly more efficient, because the move for a POD is effectively a copy.

But for config structs this should not be a concern.

BUT: we should never pass a mutable ref and move that, because it moves from the source value that the caller holds, which is super unexpected.