<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Looks like Past You has had some thoughts too: <a href="https://bugs.llvm.org/show_bug.cgi?id=17234" class="">https://bugs.llvm.org/show_bug.cgi?id=17234</a><div class=""><br class=""></div><div class="">Jordan</div><div class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Oct 14, 2019, at 16:49, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Fair enough - yeah, I think this comes back to wanting an annotation on types that are "value-like" (I've wanted this for "unused variable" warnings and the like (basically 3 classes: std::mutex_lock (where the ctor/dtor create a temporary side effect, so things like "std::mutex_lock(t), f();" are valid, even though the mutex_lock isn't named/referenced, but "std::mutex_lock(t);" is not useful, because nothing happens between construction and destruction), things like std::string (where you need to interact with it in some way), and "the rest" that have unbounded side effects).<br class=""><br class="">With such an attribute, I think you could diagnose the example you gave (& things like "std::string("foo").empty()", etc) as problematic/error-prone, since no information from the variable "Leaks" out. (it takes all parameters by const reference (and/or maybe rvalue), and nothing is returned)<br class=""><br class="">- Dave</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 14, 2019 at 4:41 PM Jordan Rose <<a href="mailto:jordan_rose@apple.com" class="">jordan_rose@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class="">I had to dig up the example again, but it was basically a nested PointerIntPair, leading to<div class=""><br class=""></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px" class=""><div class="">importantFieldAndFlags.getPointer().setInt(true); // this setInt is discarded</div></blockquote><div class=""><br class=""></div><div class="">I don't see how you could write a verifier to catch this without looking at the body of 'setInt', although maybe you could infer it from the method being named "set*".</div><div class=""><br class=""></div>Jordan<div class=""><br class=""><div class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Oct 14, 2019, at 16:34, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Hmm, actually, thinking about this again - what's the bug usage you're trying to fix? If these functions don't return any value, what sort of mistakes could they introduce?<br class=""><br class="">It's not like someone's going to accidentally write "foo(bar().baz())" (where baz() is the mutating-not-returning function)<br class=""><br class="">Is it in cases like "bar().baz()" where bar() returns by value & the author thought it returned by non-const-ref?<br class=""><br class="">I don't think the warning or tidy-check should suggest adding a reference qualifier - just catching the usage that's bogus (ie: diagnosing the "bar().baz()" usage, not the definition of "baz()" itself) - because I think there's just too much API surface area to annotate everything.</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 14, 2019 at 4:31 PM Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank" class="">jordan_rose@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="">I could see it as a clang-tidy pass. "This method modifies members of 'this' but does not return 'this' or use it in any way [after modification]; do you want to make it lvalue-only?" I wouldn't make it a full warning because lvalue qualifiers are still fairly esoteric and people may not want to add them to their codebase even if they'd catch bugs.<div class=""><br class=""></div><div class="">Jordan<br class=""><div class=""><br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Oct 14, 2019, at 15:43, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Seems like a losing race to try to flag every API surface area that might have this problem.<br class=""><br class="">Is it worth considering a clang-tidy or full clang warning for cases like this? (& could diagnose the usage directly)</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 8, 2019 at 11:59 AM Jordan Rose via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: jrose<br class="">
Date: Tue Oct 8 12:01:48 2019<br class="">
New Revision: 374102<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=374102&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=374102&view=rev</a><br class="">
Log:<br class="">
Mark several PointerIntPair methods as lvalue-only<br class="">
<br class="">
No point in mutating 'this' if it's just going to be thrown away.<br class="">
<br class="">
<a href="https://reviews.llvm.org/D63945" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D63945</a><br class="">
<br class="">
Modified:<br class="">
llvm/trunk/include/llvm/ADT/PointerIntPair.h<br class="">
<br class="">
Modified: llvm/trunk/include/llvm/ADT/PointerIntPair.h<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/PointerIntPair.h?rev=374102&r1=374101&r2=374102&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/PointerIntPair.h?rev=374102&r1=374101&r2=374102&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/include/llvm/ADT/PointerIntPair.h (original)<br class="">
+++ llvm/trunk/include/llvm/ADT/PointerIntPair.h Tue Oct 8 12:01:48 2019<br class="">
@@ -13,6 +13,7 @@<br class="">
#ifndef LLVM_ADT_POINTERINTPAIR_H<br class="">
#define LLVM_ADT_POINTERINTPAIR_H<br class="">
<br class="">
+#include "llvm/Support/Compiler.h"<br class="">
#include "llvm/Support/PointerLikeTypeTraits.h"<br class="">
#include "llvm/Support/type_traits.h"<br class="">
#include <cassert><br class="">
@@ -59,19 +60,19 @@ public:<br class="">
<br class="">
IntType getInt() const { return (IntType)Info::getInt(Value); }<br class="">
<br class="">
- void setPointer(PointerTy PtrVal) {<br class="">
+ void setPointer(PointerTy PtrVal) LLVM_LVALUE_FUNCTION {<br class="">
Value = Info::updatePointer(Value, PtrVal);<br class="">
}<br class="">
<br class="">
- void setInt(IntType IntVal) {<br class="">
+ void setInt(IntType IntVal) LLVM_LVALUE_FUNCTION {<br class="">
Value = Info::updateInt(Value, static_cast<intptr_t>(IntVal));<br class="">
}<br class="">
<br class="">
- void initWithPointer(PointerTy PtrVal) {<br class="">
+ void initWithPointer(PointerTy PtrVal) LLVM_LVALUE_FUNCTION {<br class="">
Value = Info::updatePointer(0, PtrVal);<br class="">
}<br class="">
<br class="">
- void setPointerAndInt(PointerTy PtrVal, IntType IntVal) {<br class="">
+ void setPointerAndInt(PointerTy PtrVal, IntType IntVal) LLVM_LVALUE_FUNCTION {<br class="">
Value = Info::updateInt(Info::updatePointer(0, PtrVal),<br class="">
static_cast<intptr_t>(IntVal));<br class="">
}<br class="">
@@ -89,7 +90,7 @@ public:<br class="">
<br class="">
void *getOpaqueValue() const { return reinterpret_cast<void *>(Value); }<br class="">
<br class="">
- void setFromOpaqueValue(void *Val) {<br class="">
+ void setFromOpaqueValue(void *Val) LLVM_LVALUE_FUNCTION {<br class="">
Value = reinterpret_cast<intptr_t>(Val);<br class="">
}<br class="">
<br class="">
<br class="">
<br class="">
_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class="">
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
</blockquote></div>
</div></blockquote></div><br class=""></div></div></div></blockquote></div>
</div></blockquote></div><br class=""></div></div></div></blockquote></div>
</div></blockquote></div><br class=""></div></body></html>