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

Clang Frontend
1

Hi,

I’m seeking some opinions/advice on a clang-tidy check that I’ve been working on. It seemed to me to be a no-brainer check to add, but the more I work on it, the more questions arise, and I want to get a feel for what people think about whether it belongs in clang-tidy, and in what capacity, and how it should work.

The check basically flags code such as this:
struct A {
std::string getS() const { return s; }
private:
std::string s;
};
where getS is used as an accessor for the member s, but, because of its by-value return type, expensively copies the member on each access. A sensible and most often compatible change is to adjust getS so that it returns const std::string&. Code such as:
const auto str = myA.getS();
continues to work identically as before, but code such as:
const auto size = myA.getS().size();
saves a copy-construction of a std::string.

I have the basics of this working, but as I think about it more, a few concerns arise:

  1. this breaks code that calls non-const member functions on the result of getS(), e.g. myA.getS() += 'x';. This code may be smelly, but it is legal. What’s the stance of clang-tidy on issues like this? Are checks that potentially break call sites for the sake of performance welcome or unwelcome?

  2. it becomes more difficult as you stretch the definition of what an ‘accessor’ is:
    struct A {
    std::string getS(bool a, bool b) const { return a ? x : b ? y : z; }
    private:
    std::string x, y, z;
    };
    The definition of getS can be arbitrarily complex, but as long as it always ends up returning a member, the check could kick in. In fact, why not just expand the check to apply to any function that returns a non-local? That seems perhaps too aggressive, but where is the line?

  3. Why stop at const&? The struct A above could maybe benefit from another function overloaded on &&, that returns a std::string&& by std::move()ing s. That may create an undesired moved-from state for A, but it seems no less arbitrary than point (1) above. What is clang-tidy’s general stance on being opinionated like this?

There might be more, but that’s enough for now. Interested in any feedback anyone has, and interested in any info about how such design decisions have been addressed by clang-tidy in the past.

Thanks,
Logan R. Smith

2

Would it be helpful to apply whatever the logic is that clang-tidy has for “for (X y : z)” - I think there’s a clang-tidy check for expensive copies in the for loop. Maybe that is more conservative than you want (perhaps it only kicks in for “const auto” on a large/complex type and recommends the missing &, so it doesn’t have the writability concerns, etc)?

3

LibreOffice has a version of this,
https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/passstuffbyref.cxx

which we don’t leave on all the time because of lifetime concerns.

i.e. what if we have code like

auto x = get();
… some stuff …
x.foo()

and between the get() and the foo(), the underlying value is either dead, or has changed value.

4

@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. 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–unless people had been like “oh yeah, it’s fine, submit it, we have tons of those in clang-tidy!” which they don’t seem to be saying.

Been having a lot of fun fiddling around with ASTMatchers and such. Maybe I’ll turn my sights to implementing a simpler and less risky check.

5

@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 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

6

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?

7

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. :wink:

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 p_;
std::shared_ptr&& pref() && { return std::move(p_); }
std::shared_ptr pval() && { return std::move(p_); }
};

S s;
std::weak_ptr w1 = std::move(s).pref(); // leaves s.p_ unchanged

std::weak_ptr 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