diff mbox series

warnings about unused shared_ptr/unique_ptr comparisons

Message ID f1041ef5-86c8-3a77-7965-49cb0ed18c64@redhat.com
State New
Headers show
Series warnings about unused shared_ptr/unique_ptr comparisons | expand

Commit Message

Ulrich Drepper Jan. 14, 2019, 3:53 p.m. UTC 
This is a conservative implementation of a patch to make
shared/unique_ptrs behave more like plain old pointers.  More about this
in bug #88738

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88738

The summary is

- using clang, which enables a warning for unused results of all
comparison operation, found a real bug

- a library implementation is limited in scope and tedious to add
everywhere. At this stage of gcc 9 it was the only acceptable solution,
though

- longer term there should be a warning for comparison operators.
Possibly on by default with the possibility to disable it with an
attribute (see the discussion in the bug).


The patch proposed here only changes the code for C++17 and up to use
the [[nodiscard]] attribute.  For gcc 10 we can either widen this or
implement a better way with the help of the compiler.

I ran the regression test suite and didn't see any additional failures.

OK?

libstdc++-v3/
2019-02-14  Ulrich Drepper  <drepper@redhat.com>

	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.

Comments

Kyrill Tkachov Jan. 14, 2019, 4:04 p.m. UTC | #1 
On 14/01/19 15:53, Ulrich Drepper wrote:
> This is a conservative implementation of a patch to make
> shared/unique_ptrs behave more like plain old pointers.  More about this
> in bug #88738
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88738
>
> The summary is
>
> - using clang, which enables a warning for unused results of all
> comparison operation, found a real bug
>
> - a library implementation is limited in scope and tedious to add
> everywhere. At this stage of gcc 9 it was the only acceptable solution,
> though
>
> - longer term there should be a warning for comparison operators.
> Possibly on by default with the possibility to disable it with an
> attribute (see the discussion in the bug).
>
>
> The patch proposed here only changes the code for C++17 and up to use
> the [[nodiscard]] attribute.  For gcc 10 we can either widen this or
> implement a better way with the help of the compiler.
>
> I ran the regression test suite and didn't see any additional failures.
>
> OK?

Forwarding to the libstdc++ list for these patches.

Thanks,
Kyrill

> libstdc++-v3/
> 2019-02-14  Ulrich Drepper  <drepper@redhat.com>
>
> 	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.
>
Jonathan Wakely Jan. 14, 2019, 8:41 p.m. UTC | #2 
On 14/01/19 16:53 +0100, Ulrich Drepper wrote:
>This is a conservative implementation of a patch to make
>shared/unique_ptrs behave more like plain old pointers.  More about this
>in bug #88738
>
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88738
>
>The summary is
>
>- using clang, which enables a warning for unused results of all
>comparison operation, found a real bug
>
>- a library implementation is limited in scope and tedious to add
>everywhere. At this stage of gcc 9 it was the only acceptable solution,
>though
>
>- longer term there should be a warning for comparison operators.
>Possibly on by default with the possibility to disable it with an
>attribute (see the discussion in the bug).
>
>
>The patch proposed here only changes the code for C++17 and up to use
>the [[nodiscard]] attribute.  For gcc 10 we can either widen this or
>implement a better way with the help of the compiler.
>
>I ran the regression test suite and didn't see any additional failures.
>
>OK?

As it only makes changes for C++17 and up, this is OK for trunk now.
Thanks.
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
index 9b2fabd7d76..97bb6db70b1 100644
--- a/libstdc++-v3/include/bits/c++config
+++ b/libstdc++-v3/include/bits/c++config
@@ -99,6 +99,14 @@ 
 # define _GLIBCXX_ABI_TAG_CXX11 __attribute ((__abi_tag__ ("cxx11")))
 #endif
 
+// Macro to warn about unused results.
+#if __cplusplus >= 201703L
+# define _GLIBCXX_NODISCARD [[__nodiscard__]]
+#else
+# define _GLIBCXX_NODISCARD
+#endif
+
+
 
 #if __cplusplus
 
diff --git a/libstdc++-v3/include/bits/shared_ptr.h b/libstdc++-v3/include/bits/shared_ptr.h
index 99009ab4f99..d504627d1a0 100644
--- a/libstdc++-v3/include/bits/shared_ptr.h
+++ b/libstdc++-v3/include/bits/shared_ptr.h
@@ -380,37 +380,37 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   // 20.7.2.2.7 shared_ptr comparisons
   template<typename _Tp, typename _Up>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator==(const shared_ptr<_Tp>& __a, const shared_ptr<_Up>& __b) noexcept
     { return __a.get() == __b.get(); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator==(const shared_ptr<_Tp>& __a, nullptr_t) noexcept
     { return !__a; }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator==(nullptr_t, const shared_ptr<_Tp>& __a) noexcept
     { return !__a; }
 
   template<typename _Tp, typename _Up>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator!=(const shared_ptr<_Tp>& __a, const shared_ptr<_Up>& __b) noexcept
     { return __a.get() != __b.get(); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator!=(const shared_ptr<_Tp>& __a, nullptr_t) noexcept
     { return (bool)__a; }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator!=(nullptr_t, const shared_ptr<_Tp>& __a) noexcept
     { return (bool)__a; }
 
   template<typename _Tp, typename _Up>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<(const shared_ptr<_Tp>& __a, const shared_ptr<_Up>& __b) noexcept
     {
       using _Tp_elt = typename shared_ptr<_Tp>::element_type;
@@ -420,7 +420,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<(const shared_ptr<_Tp>& __a, nullptr_t) noexcept
     {
       using _Tp_elt = typename shared_ptr<_Tp>::element_type;
@@ -428,7 +428,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<(nullptr_t, const shared_ptr<_Tp>& __a) noexcept
     {
       using _Tp_elt = typename shared_ptr<_Tp>::element_type;
@@ -436,47 +436,47 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 
   template<typename _Tp, typename _Up>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<=(const shared_ptr<_Tp>& __a, const shared_ptr<_Up>& __b) noexcept
     { return !(__b < __a); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<=(const shared_ptr<_Tp>& __a, nullptr_t) noexcept
     { return !(nullptr < __a); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<=(nullptr_t, const shared_ptr<_Tp>& __a) noexcept
     { return !(__a < nullptr); }
 
   template<typename _Tp, typename _Up>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>(const shared_ptr<_Tp>& __a, const shared_ptr<_Up>& __b) noexcept
     { return (__b < __a); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>(const shared_ptr<_Tp>& __a, nullptr_t) noexcept
     { return nullptr < __a; }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>(nullptr_t, const shared_ptr<_Tp>& __a) noexcept
     { return __a < nullptr; }
 
   template<typename _Tp, typename _Up>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>=(const shared_ptr<_Tp>& __a, const shared_ptr<_Up>& __b) noexcept
     { return !(__a < __b); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>=(const shared_ptr<_Tp>& __a, nullptr_t) noexcept
     { return !(__a < nullptr); }
 
   template<typename _Tp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>=(nullptr_t, const shared_ptr<_Tp>& __a) noexcept
     { return !(nullptr < __a); }
 
diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h
index c7eb02e86fd..61a3ef05460 100644
--- a/libstdc++-v3/include/bits/unique_ptr.h
+++ b/libstdc++-v3/include/bits/unique_ptr.h
@@ -707,41 +707,41 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator==(const unique_ptr<_Tp, _Dp>& __x,
 	       const unique_ptr<_Up, _Ep>& __y)
     { return __x.get() == __y.get(); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator==(const unique_ptr<_Tp, _Dp>& __x, nullptr_t) noexcept
     { return !__x; }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator==(nullptr_t, const unique_ptr<_Tp, _Dp>& __x) noexcept
     { return !__x; }
 
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator!=(const unique_ptr<_Tp, _Dp>& __x,
 	       const unique_ptr<_Up, _Ep>& __y)
     { return __x.get() != __y.get(); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator!=(const unique_ptr<_Tp, _Dp>& __x, nullptr_t) noexcept
     { return (bool)__x; }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator!=(nullptr_t, const unique_ptr<_Tp, _Dp>& __x) noexcept
     { return (bool)__x; }
 
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<(const unique_ptr<_Tp, _Dp>& __x,
 	      const unique_ptr<_Up, _Ep>& __y)
     {
@@ -752,67 +752,67 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<(const unique_ptr<_Tp, _Dp>& __x, nullptr_t)
     { return std::less<typename unique_ptr<_Tp, _Dp>::pointer>()(__x.get(),
 								 nullptr); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<(nullptr_t, const unique_ptr<_Tp, _Dp>& __x)
     { return std::less<typename unique_ptr<_Tp, _Dp>::pointer>()(nullptr,
 								 __x.get()); }
 
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<=(const unique_ptr<_Tp, _Dp>& __x,
 	       const unique_ptr<_Up, _Ep>& __y)
     { return !(__y < __x); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<=(const unique_ptr<_Tp, _Dp>& __x, nullptr_t)
     { return !(nullptr < __x); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator<=(nullptr_t, const unique_ptr<_Tp, _Dp>& __x)
     { return !(__x < nullptr); }
 
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>(const unique_ptr<_Tp, _Dp>& __x,
 	      const unique_ptr<_Up, _Ep>& __y)
     { return (__y < __x); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>(const unique_ptr<_Tp, _Dp>& __x, nullptr_t)
     { return std::less<typename unique_ptr<_Tp, _Dp>::pointer>()(nullptr,
 								 __x.get()); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>(nullptr_t, const unique_ptr<_Tp, _Dp>& __x)
     { return std::less<typename unique_ptr<_Tp, _Dp>::pointer>()(__x.get(),
 								 nullptr); }
 
   template<typename _Tp, typename _Dp,
 	   typename _Up, typename _Ep>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>=(const unique_ptr<_Tp, _Dp>& __x,
 	       const unique_ptr<_Up, _Ep>& __y)
     { return !(__x < __y); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>=(const unique_ptr<_Tp, _Dp>& __x, nullptr_t)
     { return !(__x < nullptr); }
 
   template<typename _Tp, typename _Dp>
-    inline bool
+    _GLIBCXX_NODISCARD inline bool
     operator>=(nullptr_t, const unique_ptr<_Tp, _Dp>& __x)
     { return !(nullptr < __x); }