Fix Result constructor

[msmk0]

Ensure correct rvalue reference forwarding and use default implementations where possible. Update the RiddersPropagator to use explicit return types. This fix some remaining compiler warning for #267.

[paulgessinger]

Other than the default -> delete fixes, I'm not sure I understand what is being 'fixed' here.

[msmk0]

Other than the default -> delete fixes, I'm not sure I understand what is being 'fixed' here.

What triggers this are return std::move(...) statements in the RidderPropagator. Naively removing them leads to issues with implicitly defined/deleted Result constructors which this tries to solve. Have a look e.g. at this build output

[paulgessinger]

Yeah sure, but it seems to me like this is behaving as expected. If you don't have the explicit move there, you get a copy. Just switching to the explicit constructor should do the trick I believe. I think this is a case where the compiler warning is simply wrong, the move is not redundant at all.

[paulgessinger]

So, I explicitly changed this from default to the explicit implementation. I don't quite remember the reason here anymore however. I vaguely remember this had to do with one of the clang builds.

[paulgessinger]

Same for the move assignment, btw.

[msmk0]

This looks exactly like the use case for defaulted constructors. There should be nothing special going on here. It might be that previous issues are related to the masked compiler warnings/errors that appeared with #267. Let me try to test this on top of #267 again.

[msmk0]

Yeah sure, but it seems to me like this is behaving as expected. If you don't have the explicit move there, you get a copy. Just switching to the explicit constructor should do the trick I believe. I think this is a case where the compiler warning is simply wrong, the move is not redundant at all.

I thought that the problem with std::move is that it might prevent copy-ellision in recent compilers. So it should never be used for local variables, as mentioned e.g. in ISOCPP F.48.

[paulgessinger]

Yeah, if we get this warning, then we have to use the explicit constructors.

[codecov]

Codecov Report

Merging #309 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
+ Coverage   48.32%   48.33%   +0.01%     
==========================================
  Files         323      323              
  Lines       16370    16370              
  Branches     7606     7604       -2     
==========================================
+ Hits         7911     7913       +2     
  Misses       3173     3173              
+ Partials     5286     5284       -2     

Impacted Files	Coverage Δ
Core/include/Acts/Utilities/Result.hpp	79.59% <100.00%> (+4.08%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c69e51...d118b03. Read the comment docs.

[msmk0]

@paulgessinger I reduced the scope of this PR. Only the minimal changes to fix the errors in #267 are retained.

[msmk0]

@paulgessinger I would suggest that we merge this in without waiting for the fixed LCG CI builds.