<div dir="ltr"><div>@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().<br></div><div><br></div>@Noel thanks for the input. Here's another example of a case whose behavior is subtly broken by this idea:<br><div><br></div><div> A globalA;</div><div> void doSomething(const std::string&); // possibly modifies globalA</div><div> ...</div><div> doSomething(globalA.getS());</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 12, 2019 at 11:48 AM Noel Grandin via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_default" style="font-family:tahoma,sans-serif">LibreOffice has a version of this,</div><div class="gmail_default" style="font-family:tahoma,sans-serif"> <a href="https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/passstuffbyref.cxx" style="font-family:Arial,Helvetica,sans-serif" target="_blank">https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/passstuffbyref.cxx</a></div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif">which we don't leave on all the time because of lifetime concerns.</div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif">i.e. what if we have code like</div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif"> auto x = get();</div><div class="gmail_default" style="font-family:tahoma,sans-serif"> .... some stuff ..</div><div class="gmail_default" style="font-family:tahoma,sans-serif"> x.foo()</div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif">and between the get() and the foo(), the underlying value is either dead, or has changed value.</div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div><div class="gmail_default" style="font-family:tahoma,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 12 Nov 2019 at 20:07, Logan Smith via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi,<br><br>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.<br><br>The check basically flags code such as this:<br>struct A {<br> std::string getS() const { return s; }<br>private:<br> std::string s;<br>};<br>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:<br> const auto str = myA.getS();<br>continues to work identically as before, but code such as:<br> const auto size = myA.getS().size();<br>saves a copy-construction of a std::string.<br><br>I have the basics of this working, but as I think about it more, a few concerns arise:<br><br>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?<br><br>2) it becomes more difficult as you stretch the definition of what an 'accessor' is:<br>struct A {<br> std::string getS(bool a, bool b) const { return a ? x : b ? y : z; }<br>private:<br> std::string x, y, z;<br>};<br>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?<br><br>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?<br><br>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.<br><br>Thanks,<br>Logan R. Smith<br></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>