[cfe-dev] [clang-tidy] performance-unnecessary-copy-getter

[Arthur O'Dwyer via cfe-dev]

On Tue, Nov 12, 2019 at 2:59 PM Logan Smith <logan.r.smith0 at gmail.com>
wrote:

> Appreciate the feedback, Arthur. Bit of a tangent, but I'm curious why
> you'd prefer `string get() && { return move(s); }` over `string&& get() &&
> { return move(s); }`. The only real difference I can see is that after
> 'your' version, the member `s` is guaranteed to be moved-from, whereas it
> might not be with the reference version?
>

[cc: Nicolai Josuttis, which I probably should have done in my previous
email :)]
I think I remember what Nico and I agreed on, but I'm even less sure that I
remember the *reasons* we agreed on it. ;)

IIRC, the main rationale was a version of the "trading off safety for
performance" argument.
- In the case of the const getter, when you make it return a reference, you
trade off some amount of safety to avoid a copy. Avoiding a copy is a big
deal, enough to tip Nico's calculus (but not mine) in favor of returning a
reference by default.
- In the case of the rvalue-ref-qualified getter, when you make it return a
reference, you trade off that same amount of safety, but this time you're
merely avoiding a move-construction. Move-constructions are supposed to be
cheap, so this benefit is not big enough to tip even Nico's calculus in
favor of return-by-reference in this case.

Also, you're probably not even avoiding the move-construction, just
delaying it. With a const getter, it's totally plausible that the caller
doesn't intend to make a copy of the value but just wants to "observe" it.
With an rvalue-ref-qualified getter, the caller is definitely saying
"Please give me ownership of (a copy of) this value." They are definitely
not going to just "observe" the value; they're going to transfer it
somewhere else.

    std::string v;
    v = obj.get();  // copy-construct and then move-assign, or take a ref
and then copy-assign? No big diff.
    v = std::move(obj).get();  // move-construct and then move-assign, or
take a ref and then move-assign? No big diff.
    std::cout << obj.get();  // copy-construct, or take a ref? Big
difference! This tips the calculus for the const getter case.
    std::cout << std::move(obj).get();  // Nobody writes this. But even if
they did: move-construct, or take a ref? No big diff.

Your point about guaranteeing a moved-from state for the member `s` is also
quite valid. Consider the difference between

    // https://godbolt.org/z/kw_52f
    struct S {
        std::shared_ptr<int> p_;
        std::shared_ptr<int>&& pref() && { return std::move(p_); }
        std::shared_ptr<int> pval() && { return std::move(p_); }
    };

    S s;
    std::weak_ptr<int> w1 = std::move(s).pref();  // leaves s.p_ unchanged
    std::weak_ptr<int> w2 = std::move(s).pval();  // nulls out s.p_

In other words, returning-by-value usually makes the program easier to
reason about. References can be sneaky (or "unsafe") in more ways than just
aliasing.

–Arthur


> On Tue, Nov 12, 2019 at 4:33 PM Arthur O'Dwyer <arthur.j.odwyer at gmail.com>
> wrote:
>
>> On Tue, Nov 12, 2019 at 11:10 AM Logan Smith via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> @David there is a lot of existing machinery I've found useful, such as
>>> tidy::matchers::isExpensiveToCopy. As for the "for (X y : z)" case, that
>>> seems simpler, since the checker could analyze all the uses of `y` within
>>> the loop body and do what's most appropriate, whereas there'd be no
>>> practical way for this check to analyze all the call sites of A::getS().
>>>
>>> @Noel thanks for the input.
>>>
>>
>> (FWIW, Noel's example with `auto x = get();` was actually fine. What
>> wouldn't be fine would be `const auto& x = get();` or `decltype(auto) x =
>> get();`. This blog post
>> <https://quuxplusone.github.io/blog/2018/04/05/lifetime-extension-grudgingly-accepted/>
>> is relevant to your interests.)
>>
>> Here's another example of a case whose behavior is subtly broken by this
>>> idea:
>>>
>>>     A globalA;
>>>     void doSomething(const std::string&); // possibly modifies globalA
>>>     ...
>>>     doSomething(globalA.getS());
>>>
>>> With the by-value return, the string parameter is copied and 'frozen',
>>> whereas with the reference version it could change mid-function with any
>>> modifications to globalA.
>>>
>>
>>> This kind of stuff is feeling more and more like a nail in the coffin of
>>> this idea. I don't feel crazy about introducing a check with such subtle
>>> implications
>>>
>>
>> FWIW, I agree. I always say that C++ prefers value semantics everywhere
>> by default: C++ likes to make implicit copies, pass by value, etc. etc. The
>> programmer can do work to remove some of those copies — pass-by-reference,
>> even return-by-reference — but every time you replace a copy by a
>> reference, you're trading off safety for performance. More incidental
>> aliasing relationships equals less safety.
>>
>> Refactoring `void foo(string a, string b)` into `void foo(const string&
>> a, const string& b)` is usually safe in practice, and anyway the programmer
>> can check the O(1) amount of code inside `foo` to ensure that the
>> transformation seems safe. The newly introduced aliasing relationship has a
>> limited scope.
>> Refactoring `string bar()` into `const string& bar()` is usually "safe
>> but confusing" in practice, and [as you already observed] there's no way
>> for the programmer to check all O(N) call-sites to see whether the
>> transformation seems safe. The newly introduced aliasing relationship has
>> unbounded scope.
>>
>> (BTW, Nico Josuttis and I had a nice discussion at CppCon 2019 about this
>> getter-return-by-reference idiom. We ended up agreeing that if you have an
>> rvalue-ref-qualified getter, it should rather return by [pr]value than by
>> rvalue reference. I believe Nico would default to return-by-const-reference
>> for regular getters; whereas I would default to return-by-value as a matter
>> of course, until benchmarks showed that the code was "too slow and safe,"
>> such that a tradeoff of safety for performance was warranted.)
>>
>> –Arthur
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20191113/ea11dddc/attachment.html>