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

[Aaron Ballman via cfe-dev]

On Wed, Apr 19, 2017 at 10:04 AM, David Blaikie <dblaikie at gmail.com> wrote:
> GCC only supports this on functions - so there's no behavior we need to be
> consistent with in that regard:
>
> warn.cpp:1:44: warning: ‘warn_unused_result’ attribute only applies to
> function types [-Wattributes]
>  struct __attribute__((warn_unused_result)) foo {
>                                             ^~~
>
> (I think we support a lot of attributes in other compiler's syntaxes even
> though those other compilers don't support the attribute themselves
> (especially eg: MSVC syntax, I think) - but I don't have much
> specific/concrete evidence to backup that claim)

Awesome, then we don't have to do anything special for the GNU spelling.

~Aaron

>
> On Wed, Apr 19, 2017 at 5:58 AM Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Tue, Apr 18, 2017 at 6:55 PM, Craig Topper via cfe-dev
>> <cfe-dev at lists.llvm.org> wrote:
>> > 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.
>>
>> Keep in mind that this attribute has three different spellings:
>>
>> * [[clang::warn_unused_result]] -- we can do whatever we want here.
>> * __attribute__((warn_unused_result)) and [[gnu:warn_unused_result]]
>> -- we have some latitude, but should prefer to behave the same way as
>> GCC.
>> * [[nodiscard]] -- we should follow what the standard says.
>>
>> For the [[nodiscard]] spelling, it sounds like this is behaving
>> incorrectly already. For the [[clang::warn_unused_result]] spelling, I
>> agree with Erich -- this seems like a bigger hammer than we need, and
>> it surely should work for nonmember operators as well as member
>> operators. I would probably diagnose on post inc/dec (and arithmetic
>> operators) because ignoring that result when the class is marked to
>> warn on unused results seems in keeping with the intent of the
>> attribute. For the GNU spellings, I'd say we should mimic whatever GCC
>> does if we think it's defensible and it isn't a burden to support that
>> way.
>>
>> ~Aaron
>>
>> >
>> > ~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
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-dev mailing list
>> > cfe-dev at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >