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

[Logan Smith via cfe-dev]

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?

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/20191112/6c50b9ca/attachment.html>