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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard 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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard via cfe-dev
In reply to this post by Tom Stellard via cfe-dev
LibreOffice has a version of this,

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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard 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 <[hidden email]> wrote:
LibreOffice has a version of this,

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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard via cfe-dev
On Tue, Nov 12, 2019 at 11:10 AM Logan Smith via cfe-dev <[hidden email]> 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 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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard via cfe-dev
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?

On Tue, Nov 12, 2019 at 4:33 PM Arthur O'Dwyer <[hidden email]> wrote:
On Tue, Nov 12, 2019 at 11:10 AM Logan Smith via cfe-dev <[hidden email]> 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 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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Tom Stellard via cfe-dev
On Tue, Nov 12, 2019 at 2:59 PM Logan Smith <[hidden email]> wrote:
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. ;)

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

    struct S {
        std::shared_ptr<int> p_;
        std::shared_ptr<int>&& pref() && { return std::move(p_); }
        std::shared_ptr<int> pval() && { return std::move(p_); }
    };

    S s;
    std::weak_ptr<int> w1 = std::move(s).pref();  // leaves s.p_ unchanged
    std::weak_ptr<int> 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


On Tue, Nov 12, 2019 at 4:33 PM Arthur O'Dwyer <[hidden email]> wrote:
On Tue, Nov 12, 2019 at 11:10 AM Logan Smith via cfe-dev <[hidden email]> 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 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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev