[cfe-dev] warn_unused_result attribute on class definition not applying to methods declared in that class

[Craig Topper via cfe-dev]

Merging Erich Keane's response here. Somehow his reply branched.


Responses inline, note that Aaron is likely thebest one to :

I think this is because of the behavior outlined here.

  /// \brief Returns true if this function or its return type has the
  /// warn_unused_result attribute. If the return type has the attribute and
  /// this function is a method of the return type's class, then false will
be
  /// returned to avoid spurious warnings on member methods such as
assignment
  /// operators.
  bool hasUnusedResultAttr() const { return getUnusedResultAttr() !=
nullptr; }
[Keane, Erich] I don't really see a situation where the exception for
spurious warnings is correct.  Assignment operators typically return a
reference, thus this is suppressed anyway.  Either way, this is a much
bigger hammer than it should use, I suspect we should just special-case the
ones we wish to suppress.  What is particularly annoying about this
suppression is that it will only work for MEMBER operators, not all
operators, so it is wrong in the first place!! Looking through operators
only the following would be subject to the warning anyway:

1- Post increment/decrement <-- warning is possibly invalid?
2- arithmetic operators  <-- warning is probably valid.

Looking through history, it seems that the suppression of the warning on
references/pointers was added AFTER this, so I wonder if simply removing
the "getCorrespondingMethodInClass" condition is the right thing?  If we
decide situation 1 above should be excepted, we should specifically except
IT, for both member and non-member.

~Craig

On Tue, Apr 18, 2017 at 9:56 AM, David Blaikie <dblaikie at gmail.com> wrote:

> *nod* maybe it could be filtered to only suppress that when the result is
> a T& but not a T value. That'd catch the APInt::trunc case, but not produce
> false positives for assignment operators..
>
> On Tue, Apr 18, 2017 at 12:05 AM Craig Topper via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> I think this is because of the behavior outlined here.
>>
>>   /// \brief Returns true if this function or its return type has the
>>   /// warn_unused_result attribute. If the return type has the attribute
>> and
>>   /// this function is a method of the return type's class, then false
>> will be
>>   /// returned to avoid spurious warnings on member methods such as
>> assignment
>>   /// operators.
>>   bool hasUnusedResultAttr() const { return getUnusedResultAttr() !=
>> nullptr; }
>>
>> ~Craig
>>
>> On Mon, Apr 17, 2017 at 11:48 PM, Craig Topper <craig.topper at gmail.com>
>> wrote:
>>
>>> It seems if warn_unused_result is put on a class definition it doesn't
>>> apply to any of the methods in that class that return the class type?
>>>
>>> This test should generate 2 warnings but only generates one.
>>>
>>> https://godbolt.org/g/kImv9k
>>>
>>> The ArrayRef and APInt classes both do this. I accidentally created a
>>> case where I didn't consume the result of APInt::trunc and received no
>>> warning.
>>>
>>> ~Craig
>>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://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/20170418/1f3e3b5d/attachment.html>