Prefer pass-by-value over pass-by-rvalue

[baylesj]

I would prefer not specifying rvalue only on the function parameter. It's similarly efficient to have a pass-by-value parameter that you can move in and out of.

If you declare as:
bool insert(ArrayIndex index, Value newValue);

You can call as
Value foo(); insert(index, std::move(foo))

Some good material on this.
http://blogs.microsoft.co.il/sasha/2014/08/21/c-sink-parameter-passing/

[cdunn2001]

I don't have any preference on rvalues. All that code came from @BillyDonahue, who might have an opinion?

[dota17]

I tried to modify Value& Value::operator=(), but before that, I'v noticed that there are some history discussions about this, which is #640 and #876, so I am not quit sure about whether or not to modify it with pass-by-value. @baylesj @BillyDonahue @cdunn2001, what's your opinions?

[BillyDonahue]

I commented on pull #1067 .

[BillyDonahue]

I'd like to get to consensus that this issue can be closed.

The append and insert of standard containers are well-designed. They provide overloads for value_type const& and for value_type&&. Json::Value is cheap but not free to move, and I don't want to incur even that cost unnecessarily (when the caller gives an rvalue argument). It's currently:

• a 64-bit word union value_,
• a word for the bits_ bitfield,
• a unique_ptr for the comments_, then
• ptrdiff_t for start_ and limit_.

So nothing expensive right now, but the Value type could become more expensive to move in the future, and I wouldn't want to get to that point and have to reintroduce the current conservative const&/&& overload set that we could have just left alone.

[baylesj]

I don't understand the concern. Let's consider three cases of objects:

1 - l-value object (e.g. a non-temporary class instance)
2 - r-value object (e.g. a temporary instance)
3 - an l-value passed as an r-value (e.g. std::move)

With a single pass by value, these objects are:
1 - copied, 2 - moved or copied as determined by compiler, 3 - moved

With value_type const& and value_type&&, these objects are:
1 - copied, 2 - moved, 3 - moved

For things where copying is actually cheaper than moving, e.g. int, pass by value is still better:

void append(int foo) {
// passes the value of foo (not a reference), which is copied or moved as appropriate.
}

Versus:

void append(const int& foo) {
// passes a reference to foo, which is then copied.
}

void append(int&& foo) {
// passes a reference to foo, which is moved.
}

Can you describe a case where having const ref and move-only methods is better?

[BillyDonahue]

Okay, let's dive in. Since we're now discussing int, I guess we're talking about the general best practices, not the specifics of Json::Value. That's good.

I don't understand the concern. Let's consider three cases of objects:

1 - l-value object (e.g. a non-temporary class instance)
2 - r-value object (e.g. a temporary instance)
3 - an l-value passed as an r-value (e.g. std::move)

With a single pass by value, these objects are:
1 - copied, 2 - moved or copied as determined by compiler, 3 - moved

With value_type const& and value_type&&, these objects are:
1 - copied, 2 - moved, 3 - moved

It's important that append is a consuming function, so we have to tally its body's cost as well.
We also have to consider the non-free destructors for the temporaries we're making.
With that in mind, let's instrument a Canary class that has all of its special functions declared but not defined, and see what calls are generated at optimization level -O2 in C++17.

https://gcc.godbolt.org/z/krvCk-

With a single pass by value, these objects are:
1 - copied, 2 - moved or copied as determined by compiler, 3 - moved

Empirical results:

1 - copy + move + dtor
(binding to object parameter is a copy).
2 - move + dtor
(binding to object parameter is elided)
3 - move + move + dtor
(binding to object parameter is move)

With value_type const& and value_type&&, these objects are:
1 - copied, 2 - moved, 3 - moved

Empirical results:

1 - copy
(binding to the const& parameter is a no-op, nothing to destroy)
2 - move + dtor
(binding to the && parameter is a no-op)
3 - move
(binding to the && parameter is a no-op, nothing to destroy)

[BillyDonahue]

I made a more thorough experiment out of it to take account of the effect of translation unit boundaries, which are at play here in Json::Value.

https://github.com/BillyDonahue/experimental/tree/make_container/container_moves

I see 9 operations in the ByValue experiments and 5 operations in the ByRef experiments.

[baylesj]

Thanks for making the experiment. I played around with it and broke up the test result for clarity's sake.

In the case of passing a plain Value (case 2), both strategies constructed a new value, move-assigned it, and destructed the old one. For const reference, byRef (e.g. const& and && declarations), the compiler was smart enough to use a single copy-assign statement, same for a move-only r-value.

In every case, byRef either matched the number of instructions or had fewer. It's definitely more performant, assuming copy/move and copy-assign/move-assign have the same complexity (which they definitely should).

I'm happy to close this issue, these results are pretty conclusive IMO.

My results for posterity:

*** Inlined Containers:
    Case 1: const reference
    byValueTest: copy, move-assign, dtor
    byRefTest: copy-assign

    Case 2: value
    byValueTest: ctor, move-assign, dtor
    byRefTest: ctor, move-assign, dtor

    Case 2a: explicitly std::moved value:
    byValueTest: ctor, move, move-assign, dtor, dtor
    byRefTest: ctor, move-assign, dtor

    Case 3: Move-only r-value
    byValueTest: move, move-assign, dtor
    byRefTest: move-assign

*** Extern Containers:
    Case 1: const reference
    byValueTest: copy, move-assign, dtor
    byRefTest: copy-assign

    Case 2: value
    byValueTest: ctor, move-assign, dtor
    byRefTest: ctor, move-assign, dtor

    Case 2a: explicitly std::moved value:
    byValueTest: ctor, move, move-assign, dtor, dtor
    byRefTest: ctor, move-assign, dtor

    Case 3: Move-only r-value
    byValueTest: move, move-assign, dtor
    byRefTest: move-assign