Make fewer copies of arrays #2114
Conversation
eltoder
force-pushed
the
feature/array-copies
branch
from
ffaa622
to
6f5cdcc
Compare
| @@ -304,7 +298,8 @@ template <class Curve> void GlobalBootstrap<Curve>::calculate() const { | |||
| Curve *ts_; | |||
| const std::vector<Real> lowerBounds_, upperBounds_; | |||
There was a problem hiding this comment.
This class is local to the function, instantiated and then destroyed at function end. You can declare lowerBounds_ and upperBounds_ as const & instead and avoid all the moves.
There was a problem hiding this comment.
Sure, but note that constructor arguments for TargetFunction then should not be declared as const&, because doing so would allow passing temporary objects. We are then storing a reference to a temporary object, which will be destroyed at the end of the statement. I declared them as const pointers instead.
There was a problem hiding this comment.
It might be an unnecessary precaution. This function is the only user of TargetFunction, nobody is going to pass temporary objects. But it's ok either way.
There was a problem hiding this comment.
It's safe at the moment, but we might refractor the code in the future to, say, use std ranges. I'd rather have a compile time error than debug random numbers coming out from bootstrapping.
There was a problem hiding this comment.
My two cents is that const reference is better syntactically and stylistically, and that any regression from future refactoring will be caught right away by the address sanitizer CI job as long as there are adequate unit tests in place.
There was a problem hiding this comment.
Yes, Google C++ style prohibits non-const references and requires using pointers to mutate arguments, so using pointers for retained arguments is very natural.
There was a problem hiding this comment.
Too bad the interface of Problem requires this to inherit from CostFunction. Otherwise, the whole thing could be a lambda and just capture what it needs. Oh well.
There was a problem hiding this comment.
Good point. We can make a templated implementation of CostFunction that wraps a functor. Then we can write this as a lambda.
| void fcn(int m, | ||
| int n, | ||
| Real* x, | ||
| Real* fvec, | ||
| int* iflag); | ||
| void jacFcn(int m, | ||
| int n, | ||
| Real* x, | ||
| Real* fjac, | ||
| int* iflag); |
There was a problem hiding this comment.
They shouldn't have been in the public interface but they are. I would deprecate them (and forward their calls to the private ones, of course).
eltoder
force-pushed
the
feature/array-copies
branch
from
c80c277
to
760fb25
Compare
No description provided.