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

[Logan Smith via cfe-dev]

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

On Tue, Nov 12, 2019 at 11:48 AM Noel Grandin via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> 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.
>
>
>
> On Tue, 12 Nov 2019 at 20:07, 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
>>
> _______________________________________________
> 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/5eb9fb9b/attachment.html>