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

[Logan Smith via cfe-dev]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20191112/650bdcc1/attachment.html>