warn_unused_result attribute on class definition not applying to methods declared in that class

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul via cfe-dev
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.


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul via cfe-dev
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 <[hidden email]> 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.


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul via cfe-dev
*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 <[hidden email]> 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 <[hidden email]> 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.


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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.


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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul via cfe-dev
On Tue, Apr 18, 2017 at 6:55 PM, Craig Topper via cfe-dev
<[hidden email]> 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 <[hidden email]> 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
>> <[hidden email]> 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 <[hidden email]>
>>> 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
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul via cfe-dev
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)

On Wed, Apr 19, 2017 at 5:58 AM Aaron Ballman <[hidden email]> wrote:
On Tue, Apr 18, 2017 at 6:55 PM, Craig Topper via cfe-dev
<[hidden email]> 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 <[hidden email]> 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
>> <[hidden email]> 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 <[hidden email]>
>>> 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
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: warn_unused_result attribute on class definition not applying to methods declared in that class

Robinson, Paul via cfe-dev
On Wed, Apr 19, 2017 at 10:04 AM, David Blaikie <[hidden email]> 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 <[hidden email]>
> wrote:
>>
>> On Tue, Apr 18, 2017 at 6:55 PM, Craig Topper via cfe-dev
>> <[hidden email]> 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 <[hidden email]>
>> > 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
>> >> <[hidden email]> 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
>> >>> <[hidden email]>
>> >>> 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
>> >>> [hidden email]
>> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Loading...