From 3893f37bcec9f0e9c8ab2c0069cfec8520591860 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 6 Aug 2021 14:30:28 -0400 Subject: [PATCH] maint(clang-tidy): Bugprone enable checks (#3166) * Enable bugprone checks * Reset delta and massage config * Start to apply bugprone fixes * try to fix minor bug * Fix later * Fix perfect forwarding bugprone * Remove nolint * undo constructor delete * Fix bugprone-perfect-forwarding again * Remove TODO * Add another nolint for bugprone-exception-escape in scoped interpreter * Fix remaining bugprone errors * Properly apply bugprone-macro-parantheses * Redo formatting and remove bugprone nolint * Add coment and revert more whitespace changes * Fix typo * Fix parsing bug * Add back comma * Fix clang-tidy issue * Apply remaining clang-tidy fixes --- .clang-tidy | 4 +++ include/pybind11/cast.h | 11 ++++---- include/pybind11/detail/common.h | 39 ++++++++++++++--------------- include/pybind11/embed.h | 36 +++++++++++++------------- include/pybind11/numpy.h | 7 ++++-- include/pybind11/pybind11.h | 2 +- tests/object.h | 6 ++++- tests/pybind11_tests.h | 9 +++---- tests/test_buffers.cpp | 6 ++++- tests/test_class.cpp | 8 +++--- tests/test_multiple_inheritance.cpp | 5 +++- tests/test_numpy_dtypes.cpp | 12 +++++---- tests/test_smart_ptr.cpp | 1 + tests/test_tagbased_polymorphic.cpp | 6 ++--- tests/test_virtual_functions.cpp | 1 + 15 files changed, 85 insertions(+), 68 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index db5077c22..c83b9b2f5 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,6 +1,7 @@ FormatStyle: file Checks: ' +*bugprone*, cppcoreguidelines-init-variables, clang-analyzer-optin.cplusplus.VirtualCall, llvm-namespace-comment, @@ -43,6 +44,9 @@ readability-static-accessed-through-instance, readability-static-definition-in-anonymous-namespace, readability-string-compare, readability-uniqueptr-delete-release, +-bugprone-exception-escape, +-bugprone-reserved-identifier, +-bugprone-unused-raii, ' CheckOptions: diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 211cf09bc..988a23695 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -102,9 +102,9 @@ public: } \ return cast(*src, policy, parent); \ } \ - operator type *() { return &value; } \ - operator type &() { return value; } \ - operator type &&() && { return std::move(value); } \ + operator type *() { return &value; } /* NOLINT(bugprone-macro-parentheses) */ \ + operator type &() { return value; } /* NOLINT(bugprone-macro-parentheses) */ \ + operator type &&() && { return std::move(value); } /* NOLINT(bugprone-macro-parentheses) */ \ template \ using cast_op_type = pybind11::detail::movable_cast_op_type @@ -145,9 +145,8 @@ public: py_value = (py_type) PyFloat_AsDouble(src.ptr()); else return false; - } else if (PyFloat_Check(src.ptr())) { - return false; - } else if (!convert && !PYBIND11_LONG_CHECK(src.ptr()) && !index_check(src.ptr())) { + } else if (PyFloat_Check(src.ptr()) + || (!convert && !PYBIND11_LONG_CHECK(src.ptr()) && !index_check(src.ptr()))) { return false; } else { handle src_or_index = src; diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 8b8f5da33..27a79bfdd 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -220,8 +220,8 @@ #define PYBIND11_BYTES_SIZE PyBytes_Size #define PYBIND11_LONG_CHECK(o) PyLong_Check(o) #define PYBIND11_LONG_AS_LONGLONG(o) PyLong_AsLongLong(o) -#define PYBIND11_LONG_FROM_SIGNED(o) PyLong_FromSsize_t((ssize_t) o) -#define PYBIND11_LONG_FROM_UNSIGNED(o) PyLong_FromSize_t((size_t) o) +#define PYBIND11_LONG_FROM_SIGNED(o) PyLong_FromSsize_t((ssize_t) (o)) +#define PYBIND11_LONG_FROM_UNSIGNED(o) PyLong_FromSize_t((size_t) (o)) #define PYBIND11_BYTES_NAME "bytes" #define PYBIND11_STRING_NAME "str" #define PYBIND11_SLICE_OBJECT PyObject @@ -356,24 +356,23 @@ extern "C" { }); } \endrst */ -#define PYBIND11_MODULE(name, variable) \ - static ::pybind11::module_::module_def \ - PYBIND11_CONCAT(pybind11_module_def_, name) PYBIND11_MAYBE_UNUSED; \ - PYBIND11_MAYBE_UNUSED \ - static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ - PYBIND11_PLUGIN_IMPL(name) { \ - PYBIND11_CHECK_PYTHON_VERSION \ - PYBIND11_ENSURE_INTERNALS_READY \ - auto m = ::pybind11::module_::create_extension_module( \ - PYBIND11_TOSTRING(name), nullptr, \ - &PYBIND11_CONCAT(pybind11_module_def_, name)); \ - try { \ - PYBIND11_CONCAT(pybind11_init_, name)(m); \ - return m.ptr(); \ - } PYBIND11_CATCH_INIT_EXCEPTIONS \ - } \ - void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable) - +#define PYBIND11_MODULE(name, variable) \ + static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name) \ + PYBIND11_MAYBE_UNUSED; \ + PYBIND11_MAYBE_UNUSED \ + static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ + PYBIND11_PLUGIN_IMPL(name) { \ + PYBIND11_CHECK_PYTHON_VERSION \ + PYBIND11_ENSURE_INTERNALS_READY \ + auto m = ::pybind11::module_::create_extension_module( \ + PYBIND11_TOSTRING(name), nullptr, &PYBIND11_CONCAT(pybind11_module_def_, name)); \ + try { \ + PYBIND11_CONCAT(pybind11_init_, name)(m); \ + return m.ptr(); \ + } \ + PYBIND11_CATCH_INIT_EXCEPTIONS \ + } \ + void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ & (variable)) PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index abb1cd3cc..6e777830f 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -45,25 +45,23 @@ }); } \endrst */ -#define PYBIND11_EMBEDDED_MODULE(name, variable) \ - static ::pybind11::module_::module_def \ - PYBIND11_CONCAT(pybind11_module_def_, name); \ - static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ - static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \ - auto m = ::pybind11::module_::create_extension_module( \ - PYBIND11_TOSTRING(name), nullptr, \ - &PYBIND11_CONCAT(pybind11_module_def_, name)); \ - try { \ - PYBIND11_CONCAT(pybind11_init_, name)(m); \ - return m.ptr(); \ - } PYBIND11_CATCH_INIT_EXCEPTIONS \ - } \ - PYBIND11_EMBEDDED_MODULE_IMPL(name) \ - ::pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \ - (PYBIND11_TOSTRING(name), \ - PYBIND11_CONCAT(pybind11_init_impl_, name)); \ - void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable) - +#define PYBIND11_EMBEDDED_MODULE(name, variable) \ + static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name); \ + static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ + static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \ + auto m = ::pybind11::module_::create_extension_module( \ + PYBIND11_TOSTRING(name), nullptr, &PYBIND11_CONCAT(pybind11_module_def_, name)); \ + try { \ + PYBIND11_CONCAT(pybind11_init_, name)(m); \ + return m.ptr(); \ + } \ + PYBIND11_CATCH_INIT_EXCEPTIONS \ + } \ + PYBIND11_EMBEDDED_MODULE_IMPL(name) \ + ::pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name)( \ + PYBIND11_TOSTRING(name), PYBIND11_CONCAT(pybind11_init_impl_, name)); \ + void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ \ + & variable) // NOLINT(bugprone-macro-parentheses) PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 7313897fe..aa8294e16 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -1551,8 +1551,11 @@ private: "pybind11::vectorize(...) requires a function with at least one vectorizable argument"); public: - template - explicit vectorize_helper(T &&f) : f(std::forward(f)) { } + template ::type>::value>> + explicit vectorize_helper(T &&f) : f(std::forward(f)) {} object operator()(typename vectorize_arg::type... args) { return run(args..., diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index aeeb3368a..54213297d 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1726,7 +1726,7 @@ struct enum_base { m_base.attr(op) = cpp_function( \ [](const object &a, const object &b) { \ if (!type::handle_of(a).is(type::handle_of(b))) \ - strict_behavior; \ + strict_behavior; /* NOLINT(bugprone-macro-parentheses) */ \ return expr; \ }, \ name(op), \ diff --git a/tests/object.h b/tests/object.h index 865a9beae..6851fc624 100644 --- a/tests/object.h +++ b/tests/object.h @@ -110,7 +110,11 @@ public: /// Overwrite this reference with another reference ref& operator=(const ref& r) { - print_copy_assigned(this, "pointer", r.m_ptr); track_copy_assigned((ref_tag*) this); + if (this == &r) { + return *this; + } + print_copy_assigned(this, "pointer", r.m_ptr); + track_copy_assigned((ref_tag *) this); if (m_ptr == r.m_ptr) return *this; diff --git a/tests/pybind11_tests.h b/tests/pybind11_tests.h index d970ba8bd..12d8a777f 100644 --- a/tests/pybind11_tests.h +++ b/tests/pybind11_tests.h @@ -23,11 +23,10 @@ public: test_initializer(const char *submodule_name, Initializer init); }; -#define TEST_SUBMODULE(name, variable) \ - void test_submodule_##name(py::module_ &); \ - test_initializer name(#name, test_submodule_##name); \ - void test_submodule_##name(py::module_ &variable) - +#define TEST_SUBMODULE(name, variable) \ + void test_submodule_##name(py::module_ &); \ + test_initializer name(#name, test_submodule_##name); \ + void test_submodule_##name(py::module_ &(variable)) /// Dummy type which is not exported anywhere -- something to trigger a conversion error struct UnregisteredType { }; diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index e77c626f8..c7e2c7df3 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -40,7 +40,11 @@ TEST_SUBMODULE(buffers, m) { } Matrix &operator=(const Matrix &s) { - print_copy_assigned(this, std::to_string(m_rows) + "x" + std::to_string(m_cols) + " matrix"); + if (this == &s) { + return *this; + } + print_copy_assigned(this, + std::to_string(m_rows) + "x" + std::to_string(m_cols) + " matrix"); delete[] m_data; m_rows = s.m_rows; m_cols = s.m_cols; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 73de2a61a..dff758392 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -492,15 +492,15 @@ using DoesntBreak5 = py::class_>; using DoesntBreak6 = py::class_, std::shared_ptr>, BreaksTramp<6>>; using DoesntBreak7 = py::class_, BreaksTramp<7>, std::shared_ptr>>; using DoesntBreak8 = py::class_, std::shared_ptr>>; -#define CHECK_BASE(N) static_assert(std::is_same>::value, \ +#define CHECK_BASE(N) static_assert(std::is_same>::value, \ "DoesntBreak" #N " has wrong type!") CHECK_BASE(1); CHECK_BASE(2); CHECK_BASE(3); CHECK_BASE(4); CHECK_BASE(5); CHECK_BASE(6); CHECK_BASE(7); CHECK_BASE(8); -#define CHECK_ALIAS(N) static_assert(DoesntBreak##N::has_alias && std::is_same>::value, \ +#define CHECK_ALIAS(N) static_assert(DoesntBreak##N::has_alias && std::is_same>::value, \ "DoesntBreak" #N " has wrong type_alias!") #define CHECK_NOALIAS(N) static_assert(!DoesntBreak##N::has_alias && std::is_void::value, \ "DoesntBreak" #N " has type alias, but shouldn't!") CHECK_ALIAS(1); CHECK_ALIAS(2); CHECK_NOALIAS(3); CHECK_ALIAS(4); CHECK_NOALIAS(5); CHECK_ALIAS(6); CHECK_ALIAS(7); CHECK_NOALIAS(8); -#define CHECK_HOLDER(N, TYPE) static_assert(std::is_same>>::value, \ +#define CHECK_HOLDER(N, TYPE) static_assert(std::is_same>>::value, \ "DoesntBreak" #N " has wrong holder_type!") CHECK_HOLDER(1, unique); CHECK_HOLDER(2, unique); CHECK_HOLDER(3, unique); CHECK_HOLDER(4, unique); CHECK_HOLDER(5, unique); CHECK_HOLDER(6, shared); CHECK_HOLDER(7, shared); CHECK_HOLDER(8, shared); @@ -510,7 +510,7 @@ CHECK_HOLDER(6, shared); CHECK_HOLDER(7, shared); CHECK_HOLDER(8, shared); // failures occurs). // We have to actually look into the type: the typedef alone isn't enough to instantiate the type: -#define CHECK_BROKEN(N) static_assert(std::is_same>::value, \ +#define CHECK_BROKEN(N) static_assert(std::is_same>::value, \ "Breaks1 has wrong type!"); //// Two holder classes: diff --git a/tests/test_multiple_inheritance.cpp b/tests/test_multiple_inheritance.cpp index d6b24d34d..b5ca298d9 100644 --- a/tests/test_multiple_inheritance.cpp +++ b/tests/test_multiple_inheritance.cpp @@ -108,7 +108,10 @@ TEST_SUBMODULE(multiple_inheritance, m) { // test_multiple_inheritance_python_many_bases - #define PYBIND11_BASEN(N) py::class_>(m, "BaseN" #N).def(py::init()).def("f" #N, [](BaseN &b) { return b.i + N; }) +#define PYBIND11_BASEN(N) \ + py::class_>(m, "BaseN" #N).def(py::init()).def("f" #N, [](BaseN &b) { \ + return b.i + (N); \ + }) PYBIND11_BASEN( 1); PYBIND11_BASEN( 2); PYBIND11_BASEN( 3); PYBIND11_BASEN( 4); PYBIND11_BASEN( 5); PYBIND11_BASEN( 6); PYBIND11_BASEN( 7); PYBIND11_BASEN( 8); PYBIND11_BASEN( 9); PYBIND11_BASEN(10); PYBIND11_BASEN(11); PYBIND11_BASEN(12); diff --git a/tests/test_numpy_dtypes.cpp b/tests/test_numpy_dtypes.cpp index 340c972db..bf4f4cee7 100644 --- a/tests/test_numpy_dtypes.cpp +++ b/tests/test_numpy_dtypes.cpp @@ -148,11 +148,13 @@ py::array mkarray_via_buffer(size_t n) { 1, { n }, { sizeof(T) })); } -#define SET_TEST_VALS(s, i) do { \ - s.bool_ = (i) % 2 != 0; \ - s.uint_ = (uint32_t) (i); \ - s.float_ = (float) (i) * 1.5f; \ - s.ldbl_ = (long double) (i) * -2.5L; } while (0) +#define SET_TEST_VALS(s, i) \ + do { \ + (s).bool_ = (i) % 2 != 0; \ + (s).uint_ = (uint32_t) (i); \ + (s).float_ = (float) (i) *1.5f; \ + (s).ldbl_ = (long double) (i) * -2.5L; \ + } while (0) template py::array_t create_recarray(size_t n) { diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 57b2d894e..eeaa44147 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -182,6 +182,7 @@ struct SharedPtrRef { struct SharedFromThisRef { struct B : std::enable_shared_from_this { B() { print_created(this); } + // NOLINTNEXTLINE(bugprone-copy-constructor-init) B(const B &) : std::enable_shared_from_this() { print_copy_created(this); } B(B &&) noexcept : std::enable_shared_from_this() { print_move_created(this); } ~B() { print_destroyed(this); } diff --git a/tests/test_tagbased_polymorphic.cpp b/tests/test_tagbased_polymorphic.cpp index 838a168d2..90f40e14c 100644 --- a/tests/test_tagbased_polymorphic.cpp +++ b/tests/test_tagbased_polymorphic.cpp @@ -86,13 +86,13 @@ std::vector> create_zoo() const std::type_info* Animal::type_of_kind(Kind kind) { switch (kind) { - case Kind::Unknown: break; - + case Kind::Unknown: case Kind::Dog: break; + case Kind::Labrador: return &typeid(Labrador); case Kind::Chihuahua: return &typeid(Chihuahua); - case Kind::LastDog: break; + case Kind::LastDog: case Kind::Cat: break; case Kind::Panther: return &typeid(Panther); case Kind::LastCat: break; diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 5280af8eb..f83a7364b 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -454,6 +454,7 @@ template class PyB_Tpl : public PyA_Tpl { public: using PyA_Tpl::PyA_Tpl; // Inherit constructors (via PyA_Tpl's inherited constructors) + // NOLINTNEXTLINE(bugprone-parent-virtual-call) int unlucky_number() override { PYBIND11_OVERRIDE(int, Base, unlucky_number, ); } double lucky_number() override { PYBIND11_OVERRIDE(double, Base, lucky_number, ); } };