From 163367da397e8597eebb732a02c10b2949d166a2 Mon Sep 17 00:00:00 2001 From: Andrew Kaster Date: Sun, 31 Oct 2021 14:52:26 -0600 Subject: [PATCH] AK: Resolve clang-tidy warnings about unusual assignment operators Either not returning *this, or in the case of Variant, not checking for self assignment. In AK::Atomic, we can't return *this due to the wrapper semantics Atomic implements. --- AK/Atomic.h | 3 +++ AK/Checked.h | 3 ++- AK/Variant.h | 20 ++++++++++++-------- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/AK/Atomic.h b/AK/Atomic.h index e1a1007b2ed..70ae6d13990 100644 --- a/AK/Atomic.h +++ b/AK/Atomic.h @@ -187,6 +187,7 @@ public: return *ret; } + // NOLINTNEXTLINE(misc-unconventional-assign-operator) We want operator= to exchange the value, so returning an object of type Atomic& here does not make sense ALWAYS_INLINE T operator=(T desired) volatile noexcept { store(desired); @@ -317,6 +318,7 @@ public: return __atomic_load_n(&m_value, order); } + // NOLINTNEXTLINE(misc-unconventional-assign-operator) We want operator= to exchange the value, so returning an object of type Atomic& here does not make sense ALWAYS_INLINE T operator=(T desired) volatile noexcept { store(desired); @@ -417,6 +419,7 @@ public: return __atomic_load_n(&m_value, order); } + // NOLINTNEXTLINE(misc-unconventional-assign-operator) We want operator= to exchange the value, so returning an object of type Atomic& here does not make sense T* operator=(T* desired) volatile noexcept { store(desired); diff --git a/AK/Checked.h b/AK/Checked.h index bac072dfbec..feba84b3efc 100644 --- a/AK/Checked.h +++ b/AK/Checked.h @@ -138,7 +138,8 @@ public: template constexpr Checked& operator=(U value) { - return *this = Checked(value); + *this = Checked(value); + return *this; } constexpr Checked& operator=(const Checked& other) = default; diff --git a/AK/Variant.h b/AK/Variant.h index dae5cd7272c..883fb2e5269 100644 --- a/AK/Variant.h +++ b/AK/Variant.h @@ -271,11 +271,13 @@ public: requires(!(IsTriviallyCopyConstructible && ...) || !(IsTriviallyDestructible && ...)) #endif { - if constexpr (!(IsTriviallyDestructible && ...)) { - Helper::delete_(m_index, m_data); + if (this != &other) { + if constexpr (!(IsTriviallyDestructible && ...)) { + Helper::delete_(m_index, m_data); + } + m_index = other.m_index; + Helper::copy_(other.m_index, other.m_data, m_data); } - m_index = other.m_index; - Helper::copy_(other.m_index, other.m_data, m_data); return *this; } @@ -284,11 +286,13 @@ public: requires(!(IsTriviallyMoveConstructible && ...) || !(IsTriviallyDestructible && ...)) #endif { - if constexpr (!(IsTriviallyDestructible && ...)) { - Helper::delete_(m_index, m_data); + if (this != &other) { + if constexpr (!(IsTriviallyDestructible && ...)) { + Helper::delete_(m_index, m_data); + } + m_index = other.m_index; + Helper::move_(other.m_index, other.m_data, m_data); } - m_index = other.m_index; - Helper::move_(other.m_index, other.m_data, m_data); return *this; }