From 5d0cb37c6ec05f45126e8ad46050f8e4701ab612 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Fri, 19 Apr 2024 11:05:17 -0400 Subject: [PATCH 1/2] [libc++] Mark scoped_lock constructors as [[nodiscard]] It's basically always a bug to discard a scoped_lock. Fixes #89388 --- libcxx/include/__mutex/unique_lock.h | 20 ++++--- libcxx/include/mutex | 16 +++--- .../diagnostics/mutex.nodiscard.verify.cpp | 53 +++++++++++++++++-- .../thread.lock.guard/nodiscard.verify.cpp | 30 ----------- 4 files changed, 70 insertions(+), 49 deletions(-) delete mode 100644 libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp diff --git a/libcxx/include/__mutex/unique_lock.h b/libcxx/include/__mutex/unique_lock.h index c27ce4b24c1ae..4a616ba51ee1c 100644 --- 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 - _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 - _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 index 12fae9a88b9d7..0d2b5914bc4fd 100644 --- 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::type _Indices; diff --git a/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp b/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp index a98eb5f142113..403514233926b 100644 --- a/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp +++ b/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp @@ -12,14 +12,57 @@ // check that functions are marked [[nodiscard]] -// clang-format off - #include +#include #include "test_macros.h" void test() { - std::mutex mutex; - std::lock_guard{mutex}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} - std::lock_guard{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{m0}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::scoped_lock{m0, m1}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::scoped_lock{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{std::adopt_lock, m0}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::scoped_lock{std::adopt_lock, m0, m1}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::scoped_lock{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 time_point; + std::chrono::milliseconds duration; + std::unique_lock other; + + // clang-format off + std::unique_lock{}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock{m}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock{m, std::defer_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock{m, std::try_to_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock{m, std::adopt_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock{m, time_point}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock{m, duration}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::unique_lock(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{m}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} + std::lock_guard{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 deleted file mode 100644 index 5fe68c83b3d9f..0000000000000 --- a/libcxx/test/libcxx/thread/thread.lock/thread.lock.guard/nodiscard.verify.cpp +++ /dev/null @@ -1,30 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -// UNSUPPORTED: no-threads - -// [[nodiscard]] isn't supported in C++03 (not even as an extension) -// UNSUPPORTED: c++03 - -// - -// template class lock_guard; - -// [[nodiscard]] explicit lock_guard(mutex_type& m); -// [[nodiscard]] lock_guard(mutex_type& m, adopt_lock_t); - -// Test that we properly apply [[nodiscard]] to lock_guard's constructors, -// which is a libc++ extension. - -#include - -void f() { - std::mutex m; - std::lock_guard{m}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} - std::lock_guard{m, std::adopt_lock}; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}} -} From e10ea307e1d2c4f0ec51e7bdc9b0b49ce881fdc8 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Thu, 25 Apr 2024 10:53:31 -0400 Subject: [PATCH 2/2] Missing include --- libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp b/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp index 403514233926b..b9890ced55bb1 100644 --- a/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp +++ b/libcxx/test/libcxx/diagnostics/mutex.nodiscard.verify.cpp @@ -14,6 +14,7 @@ #include #include +#include #include "test_macros.h"