Pass Handle by value and move (part 1)

[sweemer]

To limit the size of this PR I have started with the ql/indexes directory. Once it is merged I will submit another PR for other directories.

Besides Handle, I fixed a few std::string that were not being passed by value and moved.

[coveralls]

coverage: 72.542% (+0.001%) from 72.541% when pulling ed32de77589f0d0dea5b7cd1db35e7831080e8ba on sweemer:pass-handle-by-value-2 into af6a1a4666a9f79c1d8bff32ab937cb3540e79e8 on lballabio:master.

[lballabio]

@sweemer, on second thought I think we should revert most of this as it actually causes additional operations. If we write

    class Aonia : public OvernightIndex {
      public:
        explicit Aonia(Handle<YieldTermStructure> h = {})
        : OvernightIndex("Aonia", 0, AUDCurrency(),
                         Australia(),
                         Actual365Fixed(), std::move(h)) {}
    };

and the OvernightIndex constructor already uses the copy-and-move idiom, we end up with one copy and move in the Aonia constructor and an additional move in the OvernightIndex constructor (because the compiler seems to be smart enough to avoid another copy.) If we use a const reference in the Aonia constructor, we get only the copy and the move in the OvernightIndex constructor. I'm thinking that we should use copy-and-move only in the constructors that store the data member themselves (such as BMAIndex, for instance.) What do you think?

[sweemer]

we end up with one copy and move in the Aonia constructor

In most cases it should be possible to move the handle into the constructor of Aonia, which would result in two moves instead of a copy and a move, and so on all the way down the chain. I haven't checked all the possible compiler implementations but I still believe that this is the correct approach conceptually.

I noticed a lot of the examples don't move handles or shared_ptrs when they could, so we should probably update the examples to encourage this practice more.

[lballabio]

I might misunderstand what you're saying, but if you mean that a user should pass std::move(some handle) to the constructor, I don't think that's idiomatic, nor that we should encourage it. I think std::move belongs to class implementation and library code.

Anyway, in most realistic use cases you want to hold on to your handles so you can relink them, not move them inside an object where they're no longer accessible.

[sweemer]

Hmm in my experience it is perfectly idiomatic to move an object into a by-value constructor argument. Otherwise you're forced to make a copy even when you don't want to, which violates the "don't pay for what you don't need" principle.

Moving a value into a constructor is no different from e.g. passing a shared_ptr prvalue to the constructor of Handle, which happens all over the place in QuantLib.

If you still don't like it, then the alternative would be to provide two constructors - one taking Handle by const lvalue reference and one taking by rvalue reference - but I personally prefer the simplicity of a single constructor taking Handle by value.

[lballabio]

Optimizing copying/moving for all use cases would also require overloading the constructor, as you say, and I'd also prefer having just one. But copy-and-move is optimal when you already have to do a copy anyway, because you're initializing a data member. The Aonia constructor doesn't have to do a copy, it only passes the argument through, which is probably the reason why clang-tidy was not suggesting copy-and-move as it did for other cases (including the OvernightIndex constructor).

Anyway: I would say that our normal use case, the one we want to target for optimization, is:

RelinkableHandle<YieldTermStructure> h;
auto index = make_shared<Aonia>(h);
...
// some time later:
h.linkTo(perturbed curve);

In this case we can't move h, and having the Aonia constructor take it by copy and move it into the OvernightIndex constructor does more work than passing a const reference and letting OvernightIndex do the copy and move.

I agree that this causes more work when the handle is a temporary, or when it could be moved because it's not used afterwards. But I don't think that's our main case. Moreover, the move can't be done from Python at all.

[sweemer]

OK, I will submit a new PR to remove pass by value from all classes that pass through Handle to the eventual owner.

[lballabio]

Thanks a lot!