From b2e10cf98b6df576343dc91a640bd4171111bfc0 Mon Sep 17 00:00:00 2001 From: Pavel Kirienko Date: Tue, 27 Dec 2022 09:57:10 -0800 Subject: [PATCH] c++: remove assertion `std::is_base_of_v<>` (#6) Because Derived is not a complete type at the time of the check, the behavior of std::is_base_of<> is undefined; see https://en.cppreference.com/w/cpp/types/is_base_of. Add missing includes to the test application and bump the CI deps. --- .github/workflows/main.yml | 24 ++++++++++++------------ .idea/dictionaries/pavel.xml | 1 + CMakeLists.txt | 17 ++++++++++++++--- c++/.clang-tidy | 1 + c++/cavl.hpp | 15 +++++++-------- c++/test.cpp | 14 ++++++++------ c/.clang-tidy | 2 ++ c/cavl.h | 2 +- c/test.cpp | 26 ++++++++++++++++---------- 9 files changed, 62 insertions(+), 40 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5a2fb59..6adb92d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -10,16 +10,16 @@ jobs: toolchain: ['clang', 'gcc'] include: - toolchain: gcc - c-compiler: gcc - cxx-compiler: g++ + c-compiler: gcc-12 + cxx-compiler: g++-12 - toolchain: clang c-compiler: clang cxx-compiler: clang++ steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Dependencies - run: sudo apt install gcc-multilib g++-multilib clang-tidy-12 + run: sudo apt install gcc-multilib g++-multilib clang-tidy-14 - name: Configure CMake run: > @@ -36,7 +36,7 @@ jobs: - name: Test working-directory: ${{github.workspace}}/build - run: make test + run: make test ARGS="--verbose" optimizations: runs-on: ubuntu-latest @@ -46,13 +46,13 @@ jobs: build_type: [Release, MinSizeRel] include: - toolchain: gcc - c-compiler: gcc - cxx-compiler: g++ + c-compiler: gcc-12 + cxx-compiler: g++-12 - toolchain: clang c-compiler: clang cxx-compiler: clang++ steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Dependencies run: sudo apt install gcc-multilib g++-multilib @@ -73,15 +73,15 @@ jobs: - name: Test working-directory: ${{github.workspace}}/build - run: make test + run: make test ARGS="--verbose" style_check: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - - uses: DoozyX/clang-format-lint-action@v0.12 + - uses: actions/checkout@v3 + - uses: DoozyX/clang-format-lint-action@v0.14 with: source: '.' exclude: './unity' extensions: 'c,h,cpp,hpp' - clangFormatVersion: 12 + clangFormatVersion: 14 diff --git a/.idea/dictionaries/pavel.xml b/.idea/dictionaries/pavel.xml index 5593930..2241aff 100644 --- a/.idea/dictionaries/pavel.xml +++ b/.idea/dictionaries/pavel.xml @@ -5,6 +5,7 @@ fixedsize fontname kirienko + miscompiles nihil nodesep noninfringement diff --git a/CMakeLists.txt b/CMakeLists.txt index 6a4315b..0c05d4f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,7 +10,7 @@ set(CTEST_OUTPUT_ON_FAILURE ON) # Use -DNO_STATIC_ANALYSIS=1 to suppress static analysis. If not suppressed, the tools used here shall be available. if (NOT NO_STATIC_ANALYSIS) - find_program(clang_tidy NAMES clang-tidy-12 clang-tidy) + find_program(clang_tidy NAMES clang-tidy-14 clang-tidy) if (NOT clang_tidy) message(FATAL_ERROR "Could not locate clang-tidy") endif () @@ -18,11 +18,15 @@ if (NOT NO_STATIC_ANALYSIS) set(CMAKE_CXX_CLANG_TIDY ${clang_tidy}) endif () -find_program(clang_format NAMES clang-format-12 clang-format) +find_program(clang_format NAMES clang-format-14 clang-format) if (NOT clang_format) message(STATUS "Could not locate clang-format") else () - file(GLOB format_files ${CMAKE_CURRENT_SOURCE_DIR}/*.[ch] ${CMAKE_CURRENT_SOURCE_DIR}/*.[ch]pp) + file(GLOB format_files + ${CMAKE_CURRENT_SOURCE_DIR}/c/*.[ch]pp + ${CMAKE_CURRENT_SOURCE_DIR}/c/*.[ch] + ${CMAKE_CURRENT_SOURCE_DIR}/c++/*.[ch]pp + ) message(STATUS "Using clang-format: ${clang_format}; files: ${format_files}") add_custom_target(format COMMAND ${clang_format} -i -fallback-style=none -style=file --verbose ${format_files}) endif () @@ -43,6 +47,13 @@ target_include_directories(unity SYSTEM PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/unity target_compile_definitions(unity PUBLIC -DUNITY_SHORTHAND_AS_RAW=1 -DUNITY_OUTPUT_COLOR=1) add_executable(test_c ${CMAKE_CURRENT_SOURCE_DIR}/c/test.cpp) +if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # GCC miscompiles the test with optimizations enabled with the strict aliasing enabled + # due to the reinterpret_cast<> in a test helper. This should be fixed one day. + # This deficiency of the test suite is not expected to affect applications. + # This workaround is also not required for clang. + target_compile_options(test_c PRIVATE -fno-strict-aliasing) +endif() target_link_libraries(test_c unity) add_test("run_test_c" "test_c") diff --git a/c++/.clang-tidy b/c++/.clang-tidy index 5eabbbb..bbbf6a8 100644 --- a/c++/.clang-tidy +++ b/c++/.clang-tidy @@ -20,6 +20,7 @@ Checks: >- -readability-magic-numbers, -readability-function-size, -readability-function-cognitive-complexity, + -readability-identifier-length, -llvm-header-guard, -llvm-include-order, -cppcoreguidelines-avoid-magic-numbers, diff --git a/c++/cavl.hpp b/c++/cavl.hpp index 6548c13..f324bdb 100644 --- a/c++/cavl.hpp +++ b/c++/cavl.hpp @@ -57,13 +57,13 @@ class Node using DerivedType = Derived; // Tree nodes cannot be copied for obvious reasons. - Node(const Node&) = delete; + Node(const Node&) = delete; auto operator=(const Node&) -> Node& = delete; // They can't be moved either, but the reason is less obvious. // While we can trivially update the pointers in the adjacent nodes to keep the tree valid, // we can't update external references to the tree. This breaks the tree if one attempted to move its root node. - Node(Node&& other) = delete; + Node(Node&& other) = delete; auto operator=(Node&& other) -> Node& = delete; protected: @@ -420,7 +420,7 @@ auto Node::adjustBalance(const bool increment) noexcept -> Node* const bool r = new_bf < 0; // bf<0 if left-heavy --> right rotation is needed. const int8_t sign = r ? +1 : -1; // Positive if we are rotating right. Node* const z = lr[!r]; - CAVL_ASSERT(z != nullptr); // Heavy side cannot be empty. + CAVL_ASSERT(z != nullptr); // Heavy side cannot be empty. NOLINTNEXTLINE(clang-analyzer-core.NullDereference) if ((z->bf * sign) <= 0) // Parent and child are heavy on the same side or the child is balanced. { out = z; @@ -508,7 +508,7 @@ class Tree final ~Tree() = default; /// Trees cannot be copied. - Tree(const Tree&) = delete; + Tree(const Tree&) = delete; auto operator=(const Tree&) -> Tree& = delete; /// Trees can be easily moved in constant time. This does not actually affect the tree itself, only this object. @@ -603,7 +603,6 @@ class Tree final auto empty() const noexcept { return root_ == nullptr; } private: - static_assert(std::is_base_of_v, "Invalid usage: CRTP inheritance required"); static_assert(!std::is_polymorphic_v); static_assert(std::is_same_v, typename NodeType::TreeType>); @@ -617,10 +616,10 @@ class Tree final explicit TraversalIndicatorUpdater(const Tree& sup) noexcept : that(sup) { that.traversal_in_progress_ = true; } ~TraversalIndicatorUpdater() noexcept { that.traversal_in_progress_ = false; } - TraversalIndicatorUpdater(const TraversalIndicatorUpdater&) = delete; - TraversalIndicatorUpdater(TraversalIndicatorUpdater&&) = delete; + TraversalIndicatorUpdater(const TraversalIndicatorUpdater&) = delete; + TraversalIndicatorUpdater(TraversalIndicatorUpdater&&) = delete; auto operator=(const TraversalIndicatorUpdater&) -> TraversalIndicatorUpdater& = delete; - auto operator=(TraversalIndicatorUpdater&&) -> TraversalIndicatorUpdater& = delete; + auto operator=(TraversalIndicatorUpdater&&) -> TraversalIndicatorUpdater& = delete; private: const Tree& that; diff --git a/c++/test.cpp b/c++/test.cpp index c2eaf30..429cf4f 100644 --- a/c++/test.cpp +++ b/c++/test.cpp @@ -12,6 +12,8 @@ #include #include #include +#include +#include void setUp() {} @@ -199,7 +201,7 @@ void testManual(const std::function& factory) TreeType tr; TEST_ASSERT(tr.empty()); const auto insert = [&](const std::uint8_t i) { - std::cout << "Inserting " << int(i) << std::endl; + std::cout << "Inserting " << static_cast(i) << std::endl; const auto pred = [&](const N& v) { return t.at(i)->getValue() - v.getValue(); }; TEST_ASSERT_NULL(tr.search(pred)); TEST_ASSERT_NULL(static_cast(tr).search(pred)); @@ -789,12 +791,12 @@ class V : public cavl::Node using Self::min; using Self::max; - V() = default; - virtual ~V() = default; - V(const V&) = delete; - V(V&&) = delete; + V() = default; + virtual ~V() = default; + V(const V&) = delete; + V(V&&) = delete; V& operator=(const V&) = delete; - V& operator=(V&&) = delete; + V& operator=(V&&) = delete; [[nodiscard]] virtual auto getValue() const -> std::uint16_t = 0; diff --git a/c/.clang-tidy b/c/.clang-tidy index 89dc606..b8590bd 100644 --- a/c/.clang-tidy +++ b/c/.clang-tidy @@ -14,6 +14,7 @@ Checks: >- portability-*, readability-*, -modernize-*, + -bugprone-easily-swappable-parameters, -hicpp-deprecated-headers, -hicpp-function-size, -hicpp-no-array-decay, @@ -22,6 +23,7 @@ Checks: >- -google-readability-todo, -google-runtime-references, -google-readability-function-size, + -readability-identifier-length, -readability-avoid-const-params-in-decls, -readability-magic-numbers, -readability-function-size, diff --git a/c/cavl.h b/c/cavl.h index 7b8acf0..9b948a0 100644 --- a/c/cavl.h +++ b/c/cavl.h @@ -139,7 +139,7 @@ static inline Cavl* cavlPrivateAdjustBalance(Cavl* const x, const bool increment const bool r = new_bf < 0; // bf<0 if left-heavy --> right rotation is needed. const int8_t sign = r ? +1 : -1; // Positive if we are rotating right. Cavl* const z = x->lr[!r]; - CAVL_ASSERT(z != NULL); // Heavy side cannot be empty. + CAVL_ASSERT(z != NULL); // Heavy side cannot be empty. NOLINTNEXTLINE(clang-analyzer-core.NullDereference) if ((z->bf * sign) <= 0) // Parent and child are heavy on the same side or the child is balanced. { out = z; diff --git a/c/test.cpp b/c/test.cpp index fad3764..b4c7071 100644 --- a/c/test.cpp +++ b/c/test.cpp @@ -98,8 +98,8 @@ void remove(Node** const root, const Node* const n) template std::uint8_t getHeight(const Node* const n) { - return (n != nullptr) ? std::uint8_t(1U + std::max(getHeight(reinterpret_cast*>(n->lr[0])), - getHeight(reinterpret_cast*>(n->lr[1])))) + return (n != nullptr) ? static_cast(1U + std::max(getHeight(reinterpret_cast*>(n->lr[0])), + getHeight(reinterpret_cast*>(n->lr[1])))) : 0; } @@ -154,17 +154,21 @@ void printGraphviz(const Node* const nd) std::puts("nodesep=0.0;ranksep=0.3;splines=false;"); traverse(nd, [](const Node* const x) { const char* const fill_color = (x->bf == 0) ? "black" : ((x->bf > 0) ? "orange" : "blue"); - std::printf("%u[fillcolor=%s];", unsigned(x->value), fill_color); + std::printf("%u[fillcolor=%s];", static_cast(x->value), fill_color); }); std::puts(""); traverse(nd, [](const Node* const x) { if (x->lr[0] != nullptr) { - std::printf("%u:sw->%u:n;", unsigned(x->value), unsigned(reinterpret_cast*>(x->lr[0])->value)); + std::printf("%u:sw->%u:n;", + static_cast(x->value), + static_cast(reinterpret_cast*>(x->lr[0])->value)); } if (x->lr[1] != nullptr) { - std::printf("%u:se->%u:n;", unsigned(x->value), unsigned(reinterpret_cast*>(x->lr[1])->value)); + std::printf("%u:se->%u:n;", + static_cast(x->value), + static_cast(reinterpret_cast*>(x->lr[1])->value)); } }); std::puts("\n}"); @@ -843,8 +847,8 @@ void testRemovalA() // 1 3 7 std::puts("REMOVE 4:"); remove(&root, &t[4]); - TEST_ASSERT_EQUAL(&t[5], root); print(root); + TEST_ASSERT_EQUAL(&t[5], root); TEST_ASSERT_NULL(findBrokenBalanceFactor(root)); TEST_ASSERT_NULL(findBrokenAncestry(root)); TEST_ASSERT_EQUAL(6, checkAscension(root)); @@ -1385,12 +1389,14 @@ void testMutationRandomized() } std::puts("Randomized test finished. Final state:"); - std::printf("\tsize: %u\n", unsigned(size)); - std::printf("\tcnt_addition: %u\n", unsigned(cnt_addition)); - std::printf("\tcnt_removal: %u\n", unsigned(cnt_removal)); + std::printf("\tsize: %u\n", static_cast(size)); + std::printf("\tcnt_addition: %u\n", static_cast(cnt_addition)); + std::printf("\tcnt_removal: %u\n", static_cast(cnt_removal)); if (root != nullptr) { - std::printf("\tmin/max: %u/%u\n", unsigned(root->min()->value), unsigned(root->max()->value)); + std::printf("\tmin/max: %u/%u\n", + static_cast(root->min()->value), + static_cast(root->max()->value)); } printGraphviz(root); validate();