From ae051d13e53128aa7af2356da79a6ec082aeaf3b Mon Sep 17 00:00:00 2001 From: Miki Rozloznik Date: Fri, 6 Sep 2024 18:37:46 +0800 Subject: [PATCH] Fix some clang-tidy warnings --- runtime/src/zserio/Variant.h | 114 +++++++++++++++++++--------- runtime/test/zserio/VariantTest.cpp | 92 +++++++++++----------- 2 files changed, 128 insertions(+), 78 deletions(-) diff --git a/runtime/src/zserio/Variant.h b/runtime/src/zserio/Variant.h index 2b39c57..bbe8c5e 100644 --- a/runtime/src/zserio/Variant.h +++ b/runtime/src/zserio/Variant.h @@ -34,8 +34,12 @@ struct type_at using type = std::tuple_element_t>; }; -template -constexpr bool is_heap_allocated_v = variant_element::type>::is_heap_allocated::value; +template +using type_at_t = typename type_at(I), T...>::type; + +template +constexpr bool is_heap_allocated_v = + variant_element(I), T...>::type>::is_heap_allocated::value; template struct is_first_alloc : is_first_allocator<::std::decay_t...> @@ -94,7 +98,9 @@ class BasicVariant : public AllocatorHolder AllocatorHolder(allocator) { if constexpr (detail::is_heap_allocated_v<0, T...>) + { emplace(); // enforce no empty state like std::variant + } } /** @@ -103,11 +109,10 @@ class BasicVariant : public AllocatorHolder * \param in_place_index Index of the active element. * \param args Arguments to be forwarded for element construction. */ - template >* = nullptr, + template >* = nullptr, std::enable_if_t::value>* = nullptr> - BasicVariant(zserio::in_place_index_t, ARGS&&... args) : - m_data(std::in_place_index, std::forward(args)...) + explicit BasicVariant(zserio::in_place_index_t, ARGS&&... args) : + m_data(std::in_place_index(I)>, std::forward(args)...) {} /** @@ -117,11 +122,10 @@ class BasicVariant : public AllocatorHolder * \param allocator Allocator to be used. * \param args Arguments to be forwarded for element construction. */ - template >* = nullptr> + template >* = nullptr> BasicVariant(zserio::in_place_index_t, const ALLOC& allocator, ARGS&&... args) : AllocatorHolder(allocator), - m_data(std::in_place_index, std::forward(args)...) + m_data(std::in_place_index(I)>, std::forward(args)...) {} /** @@ -131,11 +135,10 @@ class BasicVariant : public AllocatorHolder * \param allocator Allocator to be used. * \param args Arguments to be forwarded for element construction. */ - template >* = nullptr> + template >* = nullptr> BasicVariant(zserio::in_place_index_t, const ALLOC& allocator, ARGS&&... args) : AllocatorHolder(allocator), - m_data(std::in_place_index, nullptr) + m_data(std::in_place_index(I)>, nullptr) { emplace(std::forward(args)...); } @@ -146,11 +149,10 @@ class BasicVariant : public AllocatorHolder * \param in_place_index Index of the active element. * \param args Arguments to be forwarded for element construction. */ - template >* = nullptr, + template >* = nullptr, std::enable_if_t::value>* = nullptr> - BasicVariant(zserio::in_place_index_t, ARGS&&... args) : - m_data(std::in_place_index, nullptr) + explicit BasicVariant(zserio::in_place_index_t, ARGS&&... args) : + m_data(std::in_place_index(I)>, nullptr) { emplace(std::forward(args)...); } @@ -200,7 +202,9 @@ class BasicVariant : public AllocatorHolder { clear(); if constexpr (AllocTraits::propagate_on_container_copy_assignment::value) + { set_allocator(other.get_allocator_ref()); + } copy(other); } @@ -242,8 +246,10 @@ class BasicVariant : public AllocatorHolder if (this != &other) { clear(); - if (AllocTraits::propagate_on_container_move_assignment::value) + if constexpr (AllocTraits::propagate_on_container_move_assignment::value) + { set_allocator(std::move(other.get_allocator_ref())); + } move(std::move(other)); } @@ -267,15 +273,15 @@ class BasicVariant : public AllocatorHolder decltype(auto) emplace(ARGS&&... args) { clear(); - if constexpr (detail::is_heap_allocated_v) + if constexpr (detail::is_heap_allocated_v) { - using U = typename detail::type_at::type; + using U = detail::type_at_t; U* ptr = allocateValue(std::forward(args)...); - return *m_data.template emplace(ptr); + return *m_data.template emplace(I)>(ptr); } else { - return m_data.template emplace(std::forward(args)...); + return m_data.template emplace(I)>(std::forward(args)...); } } @@ -290,31 +296,41 @@ class BasicVariant : public AllocatorHolder /** * Returns a pointer to an element at index I or nullptr if index doesn't match. */ - template (I), T...>::type> + template > U* get_if() noexcept { if (I != index() || valueless_by_exception()) + { return nullptr; - - if constexpr (detail::is_heap_allocated_v) + } + if constexpr (detail::is_heap_allocated_v) + { return std::get(I)>(m_data); + } else + { return std::addressof(std::get(I)>(m_data)); + } } /** * Returns a pointer to an element at index I or nullptr if index doesn't match. */ - template (I), T...>::type> + template > const U* get_if() const noexcept { if (I != index() || valueless_by_exception()) + { return nullptr; - - if constexpr (detail::is_heap_allocated_v) + } + if constexpr (detail::is_heap_allocated_v) + { return std::get(I)>(m_data); + } else + { return std::addressof(std::get(I)>(m_data)); + } } /** @@ -322,12 +338,15 @@ class BasicVariant : public AllocatorHolder * * \throw BadVariantAccess if the requested index doesn't match. */ - template (I), T...>::type> + template > U& get() { auto ptr = get_if(); if (!ptr) - throw BadVariantAccess("Variant: Attempt to retrieve an inactive element at index ") << size_t(I); + { + throw BadVariantAccess("Variant: Attempt to retrieve an inactive element at index ") + << static_cast(I); + } return *ptr; } @@ -336,12 +355,15 @@ class BasicVariant : public AllocatorHolder * * \throw BadVariantAccess if the requested index doesn't match. */ - template (I), T...>::type> + template > const U& get() const { auto ptr = get_if(); if (!ptr) - throw BadVariantAccess("Variant: Attempt to retrieve an inactive element at index ") << size_t(I); + { + throw BadVariantAccess("Variant: Attempt to retrieve an inactive element at index ") + << static_cast(I); + } return *ptr; } @@ -353,11 +375,13 @@ class BasicVariant : public AllocatorHolder * \throw BadVariantAccess if variant is in valueless state. */ template - auto visit(F&& fun) -> decltype(fun(std::declval::type>())) + auto visit(F&& fun) -> decltype(fun(std::declval>())) { if (valueless_by_exception()) + { throw BadVariantAccess("Variant: Cannot visit variant in valueless state"); - using R = decltype(fun(std::declval::type>())); + } + using R = decltype(fun(std::declval>())); if constexpr (std::is_same_v) { std::nullptr_t dummy; @@ -379,11 +403,13 @@ class BasicVariant : public AllocatorHolder * \throw BadVariantAccess if variant is in valueless state. */ template - auto visit(F&& fun) const -> decltype(fun(std::declval::type>())) + auto visit(F&& fun) const -> decltype(fun(std::declval>())) { if (valueless_by_exception()) + { throw BadVariantAccess("Variant: Cannot visit variant in valueless state"); - using R = decltype(fun(std::declval::type>())); + } + using R = decltype(fun(std::declval>())); if constexpr (std::is_same_v) { std::nullptr_t dummy; @@ -405,7 +431,9 @@ class BasicVariant : public AllocatorHolder bool operator==(const BasicVariant& other) const { if (index() != other.index()) + { return false; + } return equalSeq(other, std::make_index_sequence()); } @@ -427,7 +455,9 @@ class BasicVariant : public AllocatorHolder bool operator<(const BasicVariant& other) const { if (index() != other.index()) + { return index() < other.index(); + } return lessSeq(other, std::make_index_sequence()); } @@ -478,7 +508,9 @@ class BasicVariant : public AllocatorHolder void visit(F&& fun, R& returnValue) { if (I != m_data.index()) + { return; + } if constexpr (std::is_same_v) { std::forward(fun)(get()); @@ -493,7 +525,9 @@ class BasicVariant : public AllocatorHolder void visit(F&& fun, R& returnValue) const { if (I != m_data.index()) + { return; + } if constexpr (std::is_same_v) { std::forward(fun)(get()); @@ -514,7 +548,9 @@ class BasicVariant : public AllocatorHolder bool equal(const BasicVariant& other) const { if (I != m_data.index()) + { return true; + } return get() == other.get(); } @@ -528,7 +564,9 @@ class BasicVariant : public AllocatorHolder bool less(const BasicVariant& other) const { if (I != m_data.index()) + { return true; + } return get() < other.get(); } @@ -583,7 +621,9 @@ class BasicVariant : public AllocatorHolder void copy(const BasicVariant& other) { if (I != other.m_data.index()) + { return; + } emplace(other.get()); } @@ -604,7 +644,9 @@ class BasicVariant : public AllocatorHolder void move(BasicVariant&& other) { if (I != other.m_data.index()) + { return; + } if constexpr (detail::is_heap_allocated_v) { auto& ptr = std::get(other.m_data); @@ -632,7 +674,9 @@ class BasicVariant : public AllocatorHolder void clear() { if (I != m_data.index()) + { return; + } if constexpr (detail::is_heap_allocated_v) { destroyValue(std::get(m_data)); diff --git a/runtime/test/zserio/VariantTest.cpp b/runtime/test/zserio/VariantTest.cpp index d43ab4d..79c72a4 100644 --- a/runtime/test/zserio/VariantTest.cpp +++ b/runtime/test/zserio/VariantTest.cpp @@ -1,4 +1,5 @@ #include +#include #include #include "gtest/gtest.h" @@ -34,27 +35,28 @@ struct ThrowingStruct struct BigObj { - explicit BigObj(char v) + explicit BigObj(char obj) : + data() // make clang-tidy happy { - data[0] = (char)v; + data[0] = (char)obj; } char value() const { return data[0]; } - bool operator==(const BigObj& o) const + bool operator==(const BigObj& obj) const { - return data[0] == o.data[0]; + return data[0] == obj.data[0]; } - bool operator!=(const BigObj& o) const + bool operator!=(const BigObj& obj) const { - return !(*this == o); + return !(*this == obj); } - bool operator<(const BigObj& o) const + bool operator<(const BigObj& obj) const { - return data[0] < o.data[0]; + return data[0] < obj.data[0]; } - char data[30]; + std::array data; }; template @@ -150,11 +152,13 @@ TYPED_TEST(VariantTest, moveAssignmentOperator) TYPED_TEST(VariantTest, assignmentOperator) { - typename TestFixture::Variant1 var1, var2; + typename TestFixture::Variant1 var1; + typename TestFixture::Variant1 var2; var2 = var1; ASSERT_TRUE(var1 == var2); - typename TestFixture::Variant1 var3(this->allocator), var4; + typename TestFixture::Variant1 var3(this->allocator); + typename TestFixture::Variant1 var4; var4 = var3; ASSERT_TRUE(var4 == var3); } @@ -187,34 +191,35 @@ TYPED_TEST(VariantTest, getIf) TYPED_TEST(VariantTest, comparison) { - typename TestFixture::Variant1 a, b; + typename TestFixture::Variant1 aaa; + typename TestFixture::Variant1 bbb; // different indexes - a.template emplace(20); - b.template emplace("I Am"); - ASSERT_LT(a.index(), b.index()); - ASSERT_FALSE(a == b); - ASSERT_TRUE(a != b); - ASSERT_TRUE(a < b); - ASSERT_TRUE(b > a); - ASSERT_TRUE(a <= b); - ASSERT_TRUE(b >= a); - ASSERT_FALSE(b < a); - ASSERT_FALSE(b <= a); - ASSERT_FALSE(a > b); - ASSERT_FALSE(a >= b); + aaa.template emplace(20); + bbb.template emplace("I Am"); + ASSERT_LT(aaa.index(), bbb.index()); + ASSERT_FALSE(aaa == bbb); + ASSERT_TRUE(aaa != bbb); + ASSERT_TRUE(aaa < bbb); + ASSERT_TRUE(bbb > aaa); + ASSERT_TRUE(aaa <= bbb); + ASSERT_TRUE(bbb >= aaa); + ASSERT_FALSE(bbb < aaa); + ASSERT_FALSE(bbb <= aaa); + ASSERT_FALSE(aaa > bbb); + ASSERT_FALSE(aaa >= bbb); // same index - a.template emplace("Donkey"); - ASSERT_EQ(a.index(), b.index()); - ASSERT_FALSE(a == b); - ASSERT_TRUE(a != b); - ASSERT_TRUE(a < b); - ASSERT_TRUE(b > a); - ASSERT_TRUE(a <= b); - ASSERT_TRUE(b >= a); - ASSERT_FALSE(b < a); - ASSERT_FALSE(b <= a); - ASSERT_FALSE(a > b); - ASSERT_FALSE(a >= b); + aaa.template emplace("Donkey"); + ASSERT_EQ(aaa.index(), bbb.index()); + ASSERT_FALSE(aaa == bbb); + ASSERT_TRUE(aaa != bbb); + ASSERT_TRUE(aaa < bbb); + ASSERT_TRUE(bbb > aaa); + ASSERT_TRUE(aaa <= bbb); + ASSERT_TRUE(bbb >= aaa); + ASSERT_FALSE(bbb < aaa); + ASSERT_FALSE(bbb <= aaa); + ASSERT_FALSE(aaa > bbb); + ASSERT_FALSE(aaa >= bbb); } TYPED_TEST(VariantTest, visit) @@ -227,7 +232,7 @@ TYPED_TEST(VariantTest, visit) ASSERT_TRUE((std::is_same_v)); }); - var.visit(overloaded{[](std::string& i) { + var.visit(overloaded{[](std::string&) { SUCCEED(); }, [](auto&&) { @@ -235,7 +240,7 @@ TYPED_TEST(VariantTest, visit) }}); int ret = var.visit(overloaded{ - [](std::string& i) { + [](std::string&) { return 1; }, [](auto&&) { @@ -281,10 +286,11 @@ TYPED_TEST(VariantTest, exception) TYPED_TEST(VariantTest, canStoreInMap) { - std::set vs; - vs.insert({}); - std::unordered_map um; - um[{}]; + std::set vset; + vset.insert({}); + std::unordered_map vmap; + typename TestFixture::Variant1 var; + vmap[{}]; } } // namespace zserio