From 1e5050b221d2f60a7d80b66446e075c275be1345 Mon Sep 17 00:00:00 2001 From: "Maarten L. Hekkelman" Date: Mon, 5 Jan 2026 16:00:59 +0100 Subject: [PATCH] clean up bugprone and cert warnings --- .clang-tidy | 3 +++ CMakeLists.txt | 2 +- include/cif++/category.hpp | 6 +++--- include/cif++/condition.hpp | 19 +++++++------------ include/cif++/cql.hpp | 1 - include/cif++/item.hpp | 22 +++++++++++----------- include/cif++/matrix.hpp | 4 +--- include/cif++/symmetry.hpp | 5 ++--- include/cif++/text.hpp | 8 ++++---- include/cif++/utilities.hpp | 9 +-------- src/category.cpp | 2 +- src/pdb/reconstruct.cpp | 2 +- src/point.cpp | 8 ++++---- src/text.cpp | 10 ++++------ test/reconstruction-test.cpp | 2 +- test/test-main.cpp | 4 +++- test/unit-v2-test.cpp | 24 +++++++++++++----------- 17 files changed, 60 insertions(+), 71 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 21ec8dd..d3047ec 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,4 +1,7 @@ Checks: '-*, + bugprone-*, + -bugprone-easily-swappable-parameters, + cert-*, modernize*, -modernize-use-trailing-return-type, -modernize-avoid-c-arrays, diff --git a/CMakeLists.txt b/CMakeLists.txt index 3fb7fe8..4e3951c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -351,7 +351,7 @@ find_program(CLANG_TIDY_EXE "clang-tidy") if(CLANG_TIDY_EXE) message(STATUS "clang-tidy found: ${CLANG_TIDY_EXE}") - set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_EXE} --warnings-as-errors=-* --exclude-header-filter='Eigen.*') + set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY_EXE} --warnings-as-errors=-*) else() message(WARNING "clang-tidy not found!") endif() diff --git a/include/cif++/category.hpp b/include/cif++/category.hpp index abb0d67..21cd213 100644 --- a/include/cif++/category.hpp +++ b/include/cif++/category.hpp @@ -111,10 +111,10 @@ class multiple_results_error : public std::runtime_error // These should be moved elsewhere, one day. /// \cond -template +template inline constexpr bool is_optional_v = false; -template -inline constexpr bool is_optional_v> = true; +template +inline constexpr bool is_optional_v> = true; /// \endcond // -------------------------------------------------------------------- diff --git a/include/cif++/condition.hpp b/include/cif++/condition.hpp index 255e446..3db1dac 100644 --- a/include/cif++/condition.hpp +++ b/include/cif++/condition.hpp @@ -26,10 +26,11 @@ #pragma once -#include "cif++/text.hpp" #include "cif++/row.hpp" +#include "cif++/text.hpp" #include +#include #include #include #include @@ -576,7 +577,7 @@ namespace detail template key_compare_condition_impl(std::string item_name, COMP &&comp, std::string s) : m_item_name(std::move(item_name)) - , m_compare(std::move(comp)) + , m_compare(std::forward(comp)) , m_str(std::move(s)) { } @@ -652,16 +653,10 @@ namespace detail bool result = false; for (auto &f : get_category_items(c)) { - try - { - if (r[f].compare(mValue) == 0) - { - result = true; - break; - } - } - catch (...) + if (r[f].compare(mValue) == 0) { + result = true; + break; } } @@ -699,7 +694,7 @@ namespace detail break; } } - catch (...) + catch (const std::exception &ex) // NOLINT(bugprone-empty-catch) { } } diff --git a/include/cif++/cql.hpp b/include/cif++/cql.hpp index b7bb0ff..baa4a79 100644 --- a/include/cif++/cql.hpp +++ b/include/cif++/cql.hpp @@ -46,7 +46,6 @@ namespace cif::cql { class result; -class row; class transaction; class connection; diff --git a/include/cif++/item.hpp b/include/cif++/item.hpp index a86eceb..60f1906 100644 --- a/include/cif++/item.hpp +++ b/include/cif++/item.hpp @@ -179,7 +179,7 @@ class item template item(const std::string_view name, T &&value) requires (std::is_same_v) : m_name(name) - , m_value(std::move(value)) + , m_value(std::forward(value)) { } @@ -453,7 +453,7 @@ struct item_handle * @return -1, 0 or 1 */ template - [[nodiscard]] int compare(const T &value, bool icase = true) const + [[nodiscard]] int compare(const T &value, bool icase = true) const noexcept { return item_value_as::compare(*this, value, icase); } @@ -463,7 +463,7 @@ struct item_handle * return true if both are equal. */ template - [[nodiscard]] bool operator==(const T &value) const + [[nodiscard]] bool operator==(const T &value) const noexcept { // TODO: icase or not icase? return item_value_as::compare(*this, value, true) == 0; @@ -476,7 +476,7 @@ struct item_handle * return true if both are not equal. */ template - [[nodiscard]] bool operator!=(const T &value) const + [[nodiscard]] bool operator!=(const T &value) const noexcept { return not operator==(value); } @@ -583,7 +583,7 @@ struct item_handle::item_value_as an return result; } - static int compare(const item_handle &ref, const T &value, bool icase) + static int compare(const item_handle &ref, const T &value, bool icase) noexcept { int result = 0; @@ -638,7 +638,7 @@ struct item_handle::item_value_as> return result; } - static int compare(const item_handle &ref, std::optional value, bool icase) + static int compare(const item_handle &ref, std::optional value, bool icase) noexcept { if (ref.empty() and not value) return 0; @@ -663,7 +663,7 @@ struct item_handle::item_value_as>> return result; } - static int compare(const item_handle &ref, bool value, bool icase) + static int compare(const item_handle &ref, bool value, bool icase) noexcept { bool rv = convert(ref); return value && rv ? 0 @@ -681,7 +681,7 @@ struct item_handle::item_value_as return { ref.text().data(), ref.text().size() }; } - static int compare(const item_handle &ref, const char (&value)[N], bool icase) + static int compare(const item_handle &ref, const char (&value)[N], bool icase) noexcept { return icase ? cif::icompare(ref.text(), value) : ref.text().compare(value); } @@ -697,7 +697,7 @@ struct item_handle::item_value_as #include #include -#include #include -#include #include #include @@ -56,7 +54,7 @@ namespace cif * @tparam M The type of the derived class */ template -class matrix_expression +class matrix_expression // NOLINT(bugprone-crtp-constructor-accessibility) { public: [[nodiscard]] constexpr std::size_t dim_m() const { return static_cast(*this).dim_m(); } ///< Return the size (dimension) in direction m diff --git a/include/cif++/symmetry.hpp b/include/cif++/symmetry.hpp index e53ba94..0522900 100644 --- a/include/cif++/symmetry.hpp +++ b/include/cif++/symmetry.hpp @@ -35,7 +35,6 @@ #include #if defined(__cpp_impl_three_way_comparison) -# include # include #endif @@ -130,14 +129,14 @@ struct symop_data /// \brief return an int representing the value stored in the two bits at offset @a offset [[nodiscard]] inline constexpr int unpack3(int offset) const { - int result = (m_packed >> offset) bitand 0x03; + int result = static_cast((m_packed >> offset) bitand 0x03); return result == 3 ? -1 : result; } /// \brief return an int representing the value stored in the three bits at offset @a offset [[nodiscard]] inline constexpr int unpack7(int offset) const { - return (m_packed >> offset) bitand 0x07; + return static_cast((m_packed >> offset) bitand 0x07); } /// \brief return an array of 15 ints representing the values stored diff --git a/include/cif++/text.hpp b/include/cif++/text.hpp index 8ca2659..bb82d64 100644 --- a/include/cif++/text.hpp +++ b/include/cif++/text.hpp @@ -88,16 +88,16 @@ namespace cif // our own case conversion routines. /// \brief return whether string @a is equal to string @a b ignoring changes in character case -bool iequals(std::string_view a, std::string_view b); +bool iequals(std::string_view a, std::string_view b) noexcept; /// \brief compare string @a is to string @a b ignoring changes in character case -int icompare(std::string_view a, std::string_view b); +int icompare(std::string_view a, std::string_view b) noexcept; /// \brief return whether string @a is equal to string @a b ignoring changes in character case -bool iequals(const char *a, const char *b); +bool iequals(const char *a, const char *b) noexcept; /// \brief compare string @a is to string @a b ignoring changes in character case -int icompare(const char *a, const char *b); +int icompare(const char *a, const char *b) noexcept; /// \brief convert the string @a s to lower case in situ void to_lower(std::string &s); diff --git a/include/cif++/utilities.hpp b/include/cif++/utilities.hpp index 2e53dbb..3f1b510 100644 --- a/include/cif++/utilities.hpp +++ b/include/cif++/utilities.hpp @@ -138,14 +138,7 @@ namespace colour friend std::basic_ostream &operator<<( std::basic_ostream &os, const coloured_string_t &cs) { - bool use_colour = false; - - if (os.rdbuf() == std::cout.rdbuf() and isatty(STDOUT_FILENO)) - use_colour = true; - else if (os.rdbuf() == std::cerr.rdbuf() and isatty(STDERR_FILENO)) - use_colour = true; - - if (use_colour) + if ((os.rdbuf() == std::cout.rdbuf() and isatty(STDOUT_FILENO)) or (os.rdbuf() == std::cerr.rdbuf() and isatty(STDERR_FILENO))) { os << "\033[" << cs.m_fore_colour << ';' << cs.m_style << ';' << cs.m_back_colour << 'm' << cs.m_str diff --git a/src/category.cpp b/src/category.cpp index b1ba0ea..f77cee2 100644 --- a/src/category.cpp +++ b/src/category.cpp @@ -860,7 +860,7 @@ bool category::is_valid() const iv->validate_value(vi->text(), ec); - if ((bool)ec) + if (ec != std::errc{}) { m_validator->report_error(ec, m_name, m_items[cix].m_name, false); continue; diff --git a/src/pdb/reconstruct.cpp b/src/pdb/reconstruct.cpp index 4e503f6..35989bc 100644 --- a/src/pdb/reconstruct.cpp +++ b/src/pdb/reconstruct.cpp @@ -643,7 +643,7 @@ void checkAtomRecords(datablock &db) float v; auto s = row.get(item_name); - if (auto [ptr, ec] = cif::from_chars(s.data(), s.data() + s.length(), v); (bool)ec) + if (auto [ptr, ec] = cif::from_chars(s.data(), s.data() + s.length(), v); ec != std::errc{}) continue; if (s.length() < prec + 1UL or s[s.length() - prec - 1] != '.') diff --git a/src/point.cpp b/src/point.cpp index ab4e9e9..bf63e5a 100644 --- a/src/point.cpp +++ b/src/point.cpp @@ -106,9 +106,9 @@ point center_points(std::vector &Points) t.m_z += pt.m_z; } - t.m_x /= Points.size(); - t.m_y /= Points.size(); - t.m_z /= Points.size(); + t.m_x /= static_cast(Points.size()); + t.m_y /= static_cast(Points.size()); + t.m_z /= static_cast(Points.size()); for (point &pt : Points) { @@ -162,7 +162,7 @@ double RMSd(const std::vector &a, const std::vector &b) sum += d.sum(); } - return std::sqrt(sum / a.size()); + return std::sqrt(sum / static_cast(a.size())); } // The next function returns the largest solution for a quartic equation diff --git a/src/text.cpp b/src/text.cpp index 945fbbe..8ac75b1 100644 --- a/src/text.cpp +++ b/src/text.cpp @@ -61,25 +61,23 @@ const uint8_t kCharToLowerMap[256] = { // -------------------------------------------------------------------- -bool iequals(std::string_view a, std::string_view b) +bool iequals(std::string_view a, std::string_view b) noexcept { bool result = a.length() == b.length(); for (auto ai = a.begin(), bi = b.begin(); result and ai != a.end(); ++ai, ++bi) result = kCharToLowerMap[uint8_t(*ai)] == kCharToLowerMap[uint8_t(*bi)]; - // result = tolower(*ai) == tolower(*bi); return result; } -bool iequals(const char *a, const char *b) +bool iequals(const char *a, const char *b) noexcept { bool result = true; for (; result and *a and *b; ++a, ++b) result = kCharToLowerMap[uint8_t(*a)] == kCharToLowerMap[uint8_t(*b)]; - return result and *a == *b; } -int icompare(std::string_view a, std::string_view b) +int icompare(std::string_view a, std::string_view b) noexcept { int d = 0; auto ai = a.begin(), bi = b.begin(); @@ -98,7 +96,7 @@ int icompare(std::string_view a, std::string_view b) return d; } -int icompare(const char *a, const char *b) +int icompare(const char *a, const char *b) noexcept { int d = 0; diff --git a/test/reconstruction-test.cpp b/test/reconstruction-test.cpp index 3062ccf..810b4cf 100644 --- a/test/reconstruction-test.cpp +++ b/test/reconstruction-test.cpp @@ -55,7 +55,7 @@ TEST_CASE("reconstruct") std::error_code ec; CHECK_FALSE(cif::pdb::is_valid_pdbx_file(f, ec)); - CHECK((bool)ec); + CHECK(ec != std::errc{}); CHECK(cif::pdb::reconstruct_pdbx(f)); } diff --git a/test/test-main.cpp b/test/test-main.cpp index 9656250..b3b1f11 100644 --- a/test/test-main.cpp +++ b/test/test-main.cpp @@ -4,10 +4,12 @@ #include -std::filesystem::path gTestDir = std::filesystem::current_path(); +std::filesystem::path gTestDir; int main(int argc, char *argv[]) { + gTestDir = std::filesystem::current_path(); + Catch::Session session; // There must be exactly one instance // Build a new parser on top of Catch2's diff --git a/test/unit-v2-test.cpp b/test/unit-v2-test.cpp index 737f402..f6d1053 100644 --- a/test/unit-v2-test.cpp +++ b/test/unit-v2-test.cpp @@ -191,7 +191,7 @@ TEST_CASE("cc_1") float tv; const auto &[ptr, ec] = cif::from_chars(txt.data(), txt.data() + txt.length(), tv); - CHECK_FALSE((bool)ec); + CHECK(ec == std::errc{}); CHECK(tv == val); if (ch != 0) CHECK(*ptr == ch); @@ -209,7 +209,7 @@ TEST_CASE("cc_2") char buffer[64] = {}; const auto &[ptr, ec] = std::to_chars(buffer, buffer + sizeof(buffer), val, std::chars_format::fixed, prec); - CHECK_FALSE((bool)ec); + CHECK(ec == std::errc{}); // This line generates a linker error on Windows: // CHECK(buffer == test); @@ -266,9 +266,11 @@ TEST_CASE("item_1") CHECK(i2.value() == mi2.value()); CHECK(i3.value() == mi3.value()); + // NOLINTBEGIN(bugprone-use-after-move) CHECK(ci1.empty()); CHECK(ci2.empty()); CHECK(ci3.empty()); + // NOLINTEND(bugprone-use-after-move) } TEST_CASE("item_2") @@ -408,7 +410,7 @@ TEST_CASE("c_2") CHECK(not c3.empty()); CHECK(c3.size() == 3); - CHECK(c.empty()); + CHECK(c.empty()); // NOLINT(bugprone-use-after-move) CHECK(c.size() == 0); c = c3; @@ -520,9 +522,9 @@ _test.name switch (id) { - case 1: CHECK(*name == "aap"); break; - case 2: CHECK(*name == "noot"); break; - case 3: CHECK(*name == "mies"); break; + case 1: CHECK(name.value_or("") == "aap"); break; + case 2: CHECK(name.value_or("") == "noot"); break; + case 3: CHECK(name.value_or("") == "mies"); break; default: CHECK(name.has_value() == false); } } @@ -2021,7 +2023,7 @@ _test.name v = db["test"].find_first>(cif::key("id") == 1, "id"); CHECK(v.has_value()); - CHECK(*v == 1); + CHECK(*v == 1); // NOLINT(bugprone-unchecked-optional-access) v = db["test"].find_first>(cif::key("id") == 6, "id"); CHECK(not v.has_value()); @@ -3447,9 +3449,9 @@ _name switch (id) { - case 1: CHECK(*name == "aap"); break; - case 2: CHECK(*name == "noot"); break; - case 3: CHECK(*name == "mies"); break; + case 1: CHECK(name.value_or("") == "aap"); break; + case 2: CHECK(name.value_or("") == "noot"); break; + case 3: CHECK(name.value_or("") == "mies"); break; default: CHECK(name.has_value() == false); } } @@ -3526,7 +3528,7 @@ _test.value auto v = test.find1>("id"_key == 1, "value"); CHECK(v.has_value()); - CHECK(*v == 1.0f); + CHECK(*v == 1.0f); // NOLINT(bugprone-unchecked-optional-access) v = test.find1>("id"_key == 4, "value"); CHECK(v.has_value() == false);