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

[David Blaikie 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 <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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170419/caa64fc0/attachment.html>