[llvm] r374102 - Mark several PointerIntPair methods as lvalue-only
Jordan Rose via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 14 16:54:21 PDT 2019
Looks like Past You has had some thoughts too: https://bugs.llvm.org/show_bug.cgi?id=17234 <https://bugs.llvm.org/show_bug.cgi?id=17234>
Jordan
> On Oct 14, 2019, at 16:49, David Blaikie <dblaikie at gmail.com> wrote:
>
> 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).
>
> 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)
>
> - Dave
>
> On Mon, Oct 14, 2019 at 4:41 PM Jordan Rose <jordan_rose at apple.com <mailto:jordan_rose at apple.com>> wrote:
> I had to dig up the example again, but it was basically a nested PointerIntPair, leading to
>
> importantFieldAndFlags.getPointer().setInt(true); // this setInt is discarded
>
> 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*".
>
> Jordan
>
>
>> On Oct 14, 2019, at 16:34, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>
>> 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?
>>
>> It's not like someone's going to accidentally write "foo(bar().baz())" (where baz() is the mutating-not-returning function)
>>
>> Is it in cases like "bar().baz()" where bar() returns by value & the author thought it returned by non-const-ref?
>>
>> 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.
>>
>> On Mon, Oct 14, 2019 at 4:31 PM Jordan Rose <jordan_rose at apple.com <mailto:jordan_rose at apple.com>> wrote:
>> 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.
>>
>> Jordan
>>
>>
>>> On Oct 14, 2019, at 15:43, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>>
>>> Seems like a losing race to try to flag every API surface area that might have this problem.
>>>
>>> Is it worth considering a clang-tidy or full clang warning for cases like this? (& could diagnose the usage directly)
>>>
>>> On Tue, Oct 8, 2019 at 11:59 AM Jordan Rose via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>> Author: jrose
>>> Date: Tue Oct 8 12:01:48 2019
>>> New Revision: 374102
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=374102&view=rev <http://llvm.org/viewvc/llvm-project?rev=374102&view=rev>
>>> Log:
>>> Mark several PointerIntPair methods as lvalue-only
>>>
>>> No point in mutating 'this' if it's just going to be thrown away.
>>>
>>> https://reviews.llvm.org/D63945 <https://reviews.llvm.org/D63945>
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/ADT/PointerIntPair.h
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/PointerIntPair.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/PointerIntPair.h?rev=374102&r1=374101&r2=374102&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/PointerIntPair.h?rev=374102&r1=374101&r2=374102&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/PointerIntPair.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/PointerIntPair.h Tue Oct 8 12:01:48 2019
>>> @@ -13,6 +13,7 @@
>>> #ifndef LLVM_ADT_POINTERINTPAIR_H
>>> #define LLVM_ADT_POINTERINTPAIR_H
>>>
>>> +#include "llvm/Support/Compiler.h"
>>> #include "llvm/Support/PointerLikeTypeTraits.h"
>>> #include "llvm/Support/type_traits.h"
>>> #include <cassert>
>>> @@ -59,19 +60,19 @@ public:
>>>
>>> IntType getInt() const { return (IntType)Info::getInt(Value); }
>>>
>>> - void setPointer(PointerTy PtrVal) {
>>> + void setPointer(PointerTy PtrVal) LLVM_LVALUE_FUNCTION {
>>> Value = Info::updatePointer(Value, PtrVal);
>>> }
>>>
>>> - void setInt(IntType IntVal) {
>>> + void setInt(IntType IntVal) LLVM_LVALUE_FUNCTION {
>>> Value = Info::updateInt(Value, static_cast<intptr_t>(IntVal));
>>> }
>>>
>>> - void initWithPointer(PointerTy PtrVal) {
>>> + void initWithPointer(PointerTy PtrVal) LLVM_LVALUE_FUNCTION {
>>> Value = Info::updatePointer(0, PtrVal);
>>> }
>>>
>>> - void setPointerAndInt(PointerTy PtrVal, IntType IntVal) {
>>> + void setPointerAndInt(PointerTy PtrVal, IntType IntVal) LLVM_LVALUE_FUNCTION {
>>> Value = Info::updateInt(Info::updatePointer(0, PtrVal),
>>> static_cast<intptr_t>(IntVal));
>>> }
>>> @@ -89,7 +90,7 @@ public:
>>>
>>> void *getOpaqueValue() const { return reinterpret_cast<void *>(Value); }
>>>
>>> - void setFromOpaqueValue(void *Val) {
>>> + void setFromOpaqueValue(void *Val) LLVM_LVALUE_FUNCTION {
>>> Value = reinterpret_cast<intptr_t>(Val);
>>> }
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191014/02e1f7ad/attachment.html>
More information about the llvm-commits
mailing list