Bug 88738

BTW, this also applies to the "unused variable" warning as in the code below but clang doesn't warn about that either.

#include <memory>

using type = std::shared_ptr<int>;
type g;

int f(int a) {
  auto p = g;
  return a+1;
}

Annotating std::operator==(shared_ptr, nullptr_t) with attribute warn_unused_result is enough to give a warning for the test case in comment #0.  

Handling the test case in comment #1 is a bit more involved because GCC doesn't issue -Wunused-variable warnings for objects of classes with ctors/dtors, so it will likely take a compiler enhancement (say, an attribute to request such warnings for classes whose user-defined ctors and dtors have no side-effects).

I've changed the Component to libstdc++ since that's the easy part (and will let the library maintainers more quickly chime in on if it's doable and when).

Created attachment 45416 [details]
Add nodiscard support

As Martin suggested, we could indeed use existing attributes in library code to warn about some of the problems.  The code from comment #0 is real, this happened in a project of mine where I mistyped an assignment.  The warning would have pointed to the problem.

How about the following patch for a start?  This compiles cleanly on x86-64.  I haven't run the test suite to see whether it breaks some regression tests.

Also, this approach should be extended beyond shared_ptr and unique_ptr, probably to at least every single bool operatorXX(...) const.  Or even every single const member function which then of course raises the question whether the compiler should learn about this…

(In reply to Ulrich Drepper from comment #3)
> Created attachment 45416 [details]
> Add nodiscard support
> 
> As Martin suggested, we could indeed use existing attributes in library code
> to warn about some of the problems.  The code from comment #0 is real, this
> happened in a project of mine where I mistyped an assignment.  The warning
> would have pointed to the problem.
> 
> How about the following patch for a start?  This compiles cleanly on x86-64.
> I haven't run the test suite to see whether it breaks some regression tests.

Please do, but it looks good in principle.

I'd feel comfortable adding this in stage 4 if it was only enabled for C++17 and up:

// Macro to warn about unused results.
#if __cplusplus >= 201703L
# define _GLIBCXX_NODISCARD [[__nodiscard__]]
#else
# define _GLIBCXX_NODISCARD
#endif

We could then enable it for C++98/C++11/C++14 during stage 1 if no problems show up.

> Also, this approach should be extended beyond shared_ptr and unique_ptr,
> probably to at least every single bool operatorXX(...) const.  Or even every
> single const member function which then of course raises the question
> whether the compiler should learn about this…

Microsoft went quite aggressive adding [[nodiscard]] *everywhere* in their library, and ended up backing some out again. There are some subtle cases (or, depending on your point of view, users do silly things and complain about getting warnings for their unreasonable code).

I do think Clang's warning in the compiler is better than having to decorate everything explicitly.

I can see how applying the attribute to every standard library function, even const, might be excessive, but I wonder if it would make sense for the majority of them, or at least for most equality and relational operators defined by the library.  If it did we could have GCC apply it implicitly to all such functions or operators defined in namespace std, and provide a new attribute to disable it in the cases where it might not be appropriate (say no_warn_unused_result).  We could also add a #pragma to control the scope where the attribute should or should not be implicitly applied to such functions.  If you think this might be a worthwhile approach to consider let's schedule it as an enhancement for GCC 10.

(In reply to Martin Sebor from comment #5)
> If it did we could have GCC apply it implicitly to
> all such functions or operators defined in namespace std, and provide a new
> attribute to disable it in the cases where it might not be appropriate (say
> no_warn_unused_result).

I looked at what clang does and it's very simplistic.  The code below produces the output shows below.  I.e., they use the warning for all comparison operators regardless of

- namespace
- return value
- member function or not
- const-ness of operators
- visible side effects

Nevertheless people use the compiler without complaining, at least visibly.

$ clang++ -c -O -Wall u.cc
u.cc:26:5: warning: equality comparison result unused [-Wunused-comparison]
  l == r;
  ~~^~~~
u.cc:26:5: note: use '=' to turn this equality comparison into an assignment
  l == r;
    ^~
    =
u.cc:31:5: warning: inequality comparison result unused [-Wunused-comparison]
  l != r;
  ~~^~~~
u.cc:31:5: note: use '|=' to turn this inequality comparison into an or-assignment
  l != r;
    ^~
    |=
u.cc:36:5: warning: relational comparison result unused [-Wunused-comparison]
  l <= r;
  ~~^~~~
u.cc:41:5: warning: relational comparison result unused [-Wunused-comparison]
  l >= r;
  ~~^~~~
u.cc:46:5: warning: relational comparison result unused [-Wunused-comparison]
  l < r;
  ~~^~~
5 warnings generated.



~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
struct foo {
  int a;
  foo(int) : a(42) {}
  auto operator==(const foo& o) const { return a == o.a; }
  auto operator!=(foo& o) { return a == o.a; }
};

auto operator<=(const foo& l, const foo& r)
{
  return l.a == r.a;
}

auto operator>=(foo& l, foo& r)
{
  l.a |= 1;
  return l.a == r.a;
}

int operator<(foo& l, foo& r)
{
  return l.a == r.a ? 0 : l.a < r.a ? -1 : 1;
}

auto f1(foo& l, foo& r)
{
  l == r;
}

auto f2(foo& l, foo& r)
{
  l != r;
}

auto f3(foo& l, foo& r)
{
  l <= r;
}

auto f4(foo& l, foo& r)
{
  l >= r;
}

auto f5(foo& l, foo& r)
{
  l < r;
}

Yes, I would guess that clang's "dumb" warning is fine for 99% of cases, and the handful of exceptions could use #pragma to disable the warning.

But until/unless G++ gets such a warning, decorating the library in a few specific places will be an improvement.

(In reply to Jonathan Wakely from comment #7)
> Yes, I would guess that clang's "dumb" warning is fine for 99% of cases, and
> the handful of exceptions could use #pragma to disable the warning.

Or an attribute that says the opposite of nodiscard? (maydiscard? unused?)

The standard maybe_unused attribute seems suitable. If an equality or relational operator function has that attribute, don't warn if its result is unused.

So weird operators with side effects can use the attribute.

Author: drepper
Date: Wed Jan 16 08:01:22 2019
New Revision: 267964

URL: https://gcc.gnu.org/viewcvs?rev=267964&root=gcc&view=rev
Log:
PR libstdc++/88738 treat shared_ptr and unique_ptr more like plain old pointers

        PR libstdc++/88738
        Warn about unused comparisons of shared_ptr/unique_ptr
        * include/bits/c++config [_GLIBCXX_NODISCARD]: Define.
        * include/bits/shared_ptr.h: Use it for operator ==, !=,
        <, <=, >, >= for shared_ptr.
        * include/bits/unique_ptr.h: Likewise for unique_ptr.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/c++config
    trunk/libstdc++-v3/include/bits/shared_ptr.h
    trunk/libstdc++-v3/include/bits/unique_ptr.h