[libc++] Mark scoped_lock and unique_lock constructors as [[nodiscard…

…]] (llvm#89397) It's basically always a bug to discard a scoped_lock or a unique_lock. Fixes llvm#89388
xgupta · Apr 29, 2024 · 7eac39f · 7eac39f
1 parent 413f6b9
commit 7eac39f
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 49 deletions.
diff --git a/libcxx/include/__mutex/unique_lock.h b/libcxx/include/__mutex/unique_lock.h
@@ -36,26 +36,28 @@ class _LIBCPP_TEMPLATE_VIS unique_lock {
   bool __owns_;
 
 public:
-  _LIBCPP_HIDE_FROM_ABI unique_lock() _NOEXCEPT : __m_(nullptr), __owns_(false) {}
-  _LIBCPP_HIDE_FROM_ABI explicit unique_lock(mutex_type& __m) : __m_(std::addressof(__m)), __owns_(true) {
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock() _NOEXCEPT : __m_(nullptr), __owns_(false) {}
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI explicit unique_lock(mutex_type& __m)
+      : __m_(std::addressof(__m)), __owns_(true) {
     __m_->lock();
   }
 
-  _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, defer_lock_t) _NOEXCEPT
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, defer_lock_t) _NOEXCEPT
       : __m_(std::addressof(__m)),
         __owns_(false) {}
 
-  _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, try_to_lock_t)
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, try_to_lock_t)
       : __m_(std::addressof(__m)), __owns_(__m.try_lock()) {}
 
-  _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, adopt_lock_t) : __m_(std::addressof(__m)), __owns_(true) {}
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, adopt_lock_t)
+      : __m_(std::addressof(__m)), __owns_(true) {}
 
   template <class _Clock, class _Duration>
-  _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, const chrono::time_point<_Clock, _Duration>& __t)
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, const chrono::time_point<_Clock, _Duration>& __t)
       : __m_(std::addressof(__m)), __owns_(__m.try_lock_until(__t)) {}
 
   template <class _Rep, class _Period>
-  _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, const chrono::duration<_Rep, _Period>& __d)
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock(mutex_type& __m, const chrono::duration<_Rep, _Period>& __d)
       : __m_(std::addressof(__m)), __owns_(__m.try_lock_for(__d)) {}
 
   _LIBCPP_HIDE_FROM_ABI ~unique_lock() {
@@ -66,7 +68,9 @@ class _LIBCPP_TEMPLATE_VIS unique_lock {
   unique_lock(unique_lock const&)            = delete;
   unique_lock& operator=(unique_lock const&) = delete;
 
-  _LIBCPP_HIDE_FROM_ABI unique_lock(unique_lock&& __u) _NOEXCEPT : __m_(__u.__m_), __owns_(__u.__owns_) {
+  _LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI unique_lock(unique_lock&& __u) _NOEXCEPT
+      : __m_(__u.__m_),
+        __owns_(__u.__owns_) {
     __u.__m_    = nullptr;
     __u.__owns_ = false;
   }

diff --git a/libcxx/include/mutex b/libcxx/include/mutex
@@ -427,10 +427,10 @@ class _LIBCPP_TEMPLATE_VIS scoped_lock;
 template <>
 class _LIBCPP_TEMPLATE_VIS scoped_lock<> {
 public:
-  explicit scoped_lock() {}
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit scoped_lock() {}
   ~scoped_lock() = default;
 
-  _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(adopt_lock_t) {}
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(adopt_lock_t) {}
 
   scoped_lock(scoped_lock const&)            = delete;
   scoped_lock& operator=(scoped_lock const&) = delete;
@@ -445,13 +445,15 @@ private:
   mutex_type& __m_;
 
 public:
-  explicit scoped_lock(mutex_type& __m) _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability(__m)) : __m_(__m) {
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(mutex_type& __m)
+      _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability(__m))
+      : __m_(__m) {
     __m_.lock();
   }
 
   ~scoped_lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability()) { __m_.unlock(); }
 
-  _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(adopt_lock_t, mutex_type& __m)
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(adopt_lock_t, mutex_type& __m)
       _LIBCPP_THREAD_SAFETY_ANNOTATION(requires_capability(__m))
       : __m_(__m) {}
 
@@ -465,9 +467,11 @@ class _LIBCPP_TEMPLATE_VIS scoped_lock {
   typedef tuple<_MArgs&...> _MutexTuple;
 
 public:
-  _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(_MArgs&... __margs) : __t_(__margs...) { std::lock(__margs...); }
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI explicit scoped_lock(_MArgs&... __margs) : __t_(__margs...) {
+    std::lock(__margs...);
+  }
 
-  _LIBCPP_HIDE_FROM_ABI scoped_lock(adopt_lock_t, _MArgs&... __margs) : __t_(__margs...) {}
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI scoped_lock(adopt_lock_t, _MArgs&... __margs) : __t_(__margs...) {}
 
   _LIBCPP_HIDE_FROM_ABI ~scoped_lock() {
     typedef typename __make_tuple_indices<sizeof...(_MArgs)>::type _Indices;

diff --git a/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp b/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp
@@ -12,14 +12,58 @@
 
 // check that <mutex> functions are marked [[nodiscard]]
 
-// clang-format off
-
 #include <mutex>
+#include <chrono>
+#include <utility>
 
 #include "test_macros.h"
 
 void test() {
-  std::mutex mutex;
-  std::lock_guard<std::mutex>{mutex};                  // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
-  std::lock_guard<std::mutex>{mutex, std::adopt_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+  // std::scoped_lock
+  {
+#if TEST_STD_VER >= 17
+    using M = std::mutex;
+    M m0, m1, m2;
+    // clang-format off
+    std::scoped_lock<>{};                                   // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::scoped_lock<M>{m0};                                // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::scoped_lock<M, M>{m0, m1};                         // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::scoped_lock<M, M, M>{m0, m1, m2};                  // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+
+    std::scoped_lock<>{std::adopt_lock};                    // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::scoped_lock<M>{std::adopt_lock, m0};               // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::scoped_lock<M, M>{std::adopt_lock, m0, m1};        // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::scoped_lock<M, M, M>{std::adopt_lock, m0, m1, m2}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    // clang-format on
+#endif
+  }
+
+  // std::unique_lock
+  {
+    using M = std::timed_mutex; // necessary for the time_point and duration constructors
+    M m;
+    std::chrono::time_point<std::chrono::steady_clock> time_point;
+    std::chrono::milliseconds duration;
+    std::unique_lock<M> other;
+
+    // clang-format off
+    std::unique_lock<M>{};                      // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>{m};                     // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>{m, std::defer_lock};    // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>{m, std::try_to_lock};   // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>{m, std::adopt_lock};    // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>{m, time_point};         // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>{m, duration};           // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::unique_lock<M>(std::move(other));      // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    // clang-format on
+  }
+
+  // std::lock_guard
+  {
+    std::mutex m;
+    // clang-format off
+    std::lock_guard<std::mutex>{m};                  // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    std::lock_guard<std::mutex>{m, std::adopt_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
+    // clang-format on
+  }
 }
diff --git a/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp b/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp