emplace_back instead of push_back

[taion]

This is horribly trivial, but is there a particular reason not to use emplace_back instead of push_back here?

cppwamp/cppwamp/include/cppwamp/internal/varianttuple.ipp

Line 46 in 8486792

array.push_back(Variant());

I just happened to notice it while looking at #55.

[ecorm]

Since push_back is taking an r-value reference to the container's value type, there's nothing to be gained by using emplace_back. Note that vector provides a push_back overload that takes an r-value reference.

emplace_back is useful when you want to "directly" construct the new element with one or more arguments. For example, if that line had been:

array.push_back(3)

that would construct a temporary Variant from the argument 3, and then initialize the new element in the vector. In that scenario, it would have been better to use:

array.emplace_back(3)

This SO answer confirms my reasoning.

P.S.: I might take a couple days before I get the chance to go through your PR. Thanks for the contribution!

[taion]

With emplace_back, I think you'd write that line as:

array.emplace_back();

which would create the Variant in-place instead of moving in the temporary, per the SO answer.

[ecorm]

Ah, yes. That makes sense. Even if push_back or emplace_back were equivalent in this case, I think I'll change it to emplace_back anyway, to avoid this question being asked again.

If you end up making another commit in your PR, you may go ahead and change it. This change is too trivial to warrant a separate PR.

[taion]

On second thought, shouldn't this just be

    array.emplace_back(std::get<N>(std::move(tuple)));
    assignFromTuple<N+1, Ts...>(array, std::move(tuple));

I see the tests cover the toArray function, and they still pass with this change. Am I missing something?

[ecorm]

Yes, you're absolutely right. I had not looked at the "bigger picture" when you first raised the issue. Thanks for catching this. Can you slip that in your PR?