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

[David Blaikie via cfe-dev]

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)?

On Tue, Nov 12, 2019 at 10:07 AM Logan Smith via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> 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
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20191112/5decfdb4/attachment.html>