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

[Arthur O'Dwyer via cfe-dev]

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/a961d4e6/attachment.html>