From dd5354ecc436a1c853743f137fe4f7053258c021 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 10 Feb 2022 11:50:06 -0500 Subject: [PATCH 1/4] Add clang-tidy prefer-member-initializer --- .clang-tidy | 3 ++- include/pybind11/buffer_info.h | 2 ++ include/pybind11/gil.h | 1 + tests/test_buffers.cpp | 4 +++- tests/test_copy_move.cpp | 12 ++++++++++-- tests/test_factory_constructors.cpp | 27 ++++++++++++++++++++------ tests/test_sequences_and_iterators.cpp | 7 +++++-- tests/test_smart_ptr.cpp | 3 +-- tests/test_virtual_functions.cpp | 12 +++--------- 9 files changed, 48 insertions(+), 23 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 3818d3ab77..3d80f4fe72 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -5,6 +5,7 @@ Checks: ' cppcoreguidelines-init-variables, cppcoreguidelines-pro-type-static-cast-downcast, cppcoreguidelines-slicing, +clang-analyzer-optin.performance.Padding, clang-analyzer-optin.cplusplus.VirtualCall, google-explicit-constructor, llvm-namespace-comment, @@ -65,6 +66,6 @@ CheckOptions: - key: readability-implicit-bool-conversion.AllowPointerConditions value: true -HeaderFilterRegex: 'pybind11/.*h' +HeaderFilterRegex: '(^(include/pybind11/.*h | tests/*.h))' WarningsAsErrors: '*' diff --git a/include/pybind11/buffer_info.h b/include/pybind11/buffer_info.h index 1dd6955527..052551ff30 100644 --- a/include/pybind11/buffer_info.h +++ b/include/pybind11/buffer_info.h @@ -89,7 +89,9 @@ struct buffer_info { ? std::vector(view->strides, view->strides + view->ndim) : detail::c_strides({view->shape, view->shape + view->ndim}, view->itemsize), (view->readonly != 0)) { + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) this->m_view = view; + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) this->ownview = ownview; } diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 22864adb20..557823a5ac 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -135,6 +135,7 @@ class gil_scoped_release { // `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an // initialization race could occur as multiple threads try `gil_scoped_acquire`. auto &internals = detail::get_internals(); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) tstate = PyEval_SaveThread(); if (disassoc) { // Python >= 3.7 can remove this, it's an int before 3.7 diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index cedbb73eac..3a4d304676 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -17,12 +17,14 @@ TEST_SUBMODULE(buffers, m) { public: Matrix(py::ssize_t rows, py::ssize_t cols) : m_rows(rows), m_cols(cols) { print_created(this, std::to_string(m_rows) + "x" + std::to_string(m_cols) + " matrix"); - m_data = new float[(size_t) (rows*cols)]; + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + m_data = new float[(size_t) (rows * cols)]; memset(m_data, 0, sizeof(float) * (size_t) (rows * cols)); } Matrix(const Matrix &s) : m_rows(s.m_rows), m_cols(s.m_cols) { print_copy_created(this, std::to_string(m_rows) + "x" + std::to_string(m_cols) + " matrix"); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) m_data = new float[(size_t) (m_rows * m_cols)]; memcpy(m_data, s.m_data, sizeof(float) * (size_t) (m_rows * m_cols)); } diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 850edcb4d9..007b96c5b3 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -66,7 +66,11 @@ class MoveOrCopyInt { std::swap(value, m.value); return *this; } - MoveOrCopyInt(const MoveOrCopyInt &c) { print_copy_created(this, c.value); value = c.value; } + MoveOrCopyInt(const MoveOrCopyInt &c) { + print_copy_created(this, c.value); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + value = c.value; + } MoveOrCopyInt &operator=(const MoveOrCopyInt &c) { print_copy_assigned(this, c.value); value = c.value; return *this; } ~MoveOrCopyInt() { print_destroyed(this); } @@ -76,7 +80,11 @@ class CopyOnlyInt { public: CopyOnlyInt() { print_default_created(this); } explicit CopyOnlyInt(int v) : value{v} { print_created(this, value); } - CopyOnlyInt(const CopyOnlyInt &c) { print_copy_created(this, c.value); value = c.value; } + CopyOnlyInt(const CopyOnlyInt &c) { + print_copy_created(this, c.value); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + value = c.value; + } CopyOnlyInt &operator=(const CopyOnlyInt &c) { print_copy_assigned(this, c.value); value = c.value; return *this; } ~CopyOnlyInt() { print_destroyed(this); } diff --git a/tests/test_factory_constructors.cpp b/tests/test_factory_constructors.cpp index 660e2896af..455c75d536 100644 --- a/tests/test_factory_constructors.cpp +++ b/tests/test_factory_constructors.cpp @@ -38,8 +38,7 @@ class TestFactory2 { explicit TestFactory2(std::string v) : value(std::move(v)) { print_created(this, value); } public: - TestFactory2(TestFactory2 &&m) noexcept { - value = std::move(m.value); + TestFactory2(TestFactory2 &&m) noexcept : value{std::move(m.value)} { print_move_created(this); } TestFactory2 &operator=(TestFactory2 &&m) noexcept { @@ -59,8 +58,7 @@ class TestFactory3 { public: explicit TestFactory3(std::string v) : value(std::move(v)) { print_created(this, value); } - TestFactory3(TestFactory3 &&m) noexcept { - value = std::move(m.value); + TestFactory3(TestFactory3 &&m) noexcept : value{std::move(m.value)} { print_move_created(this); } TestFactory3 &operator=(TestFactory3 &&m) noexcept { @@ -93,10 +91,18 @@ class TestFactory6 { explicit TestFactory6(int i) : value{i} { print_created(this, i); } TestFactory6(TestFactory6 &&f) noexcept { print_move_created(this); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + value = f.value; + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + alias = f.alias; + } + TestFactory6(const TestFactory6 &f) { + print_copy_created(this); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) value = f.value; + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) alias = f.alias; } - TestFactory6(const TestFactory6 &f) { print_copy_created(this); value = f.value; alias = f.alias; } virtual ~TestFactory6() { print_destroyed(this); } virtual int get() { return value; } bool has_alias() const { return alias; } @@ -131,10 +137,18 @@ class TestFactory7 { explicit TestFactory7(int i) : value{i} { print_created(this, i); } TestFactory7(TestFactory7 &&f) noexcept { print_move_created(this); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + value = f.value; + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + alias = f.alias; + } + TestFactory7(const TestFactory7 &f) { + print_copy_created(this); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) value = f.value; + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) alias = f.alias; } - TestFactory7(const TestFactory7 &f) { print_copy_created(this); value = f.value; alias = f.alias; } virtual ~TestFactory7() { print_destroyed(this); } virtual int get() { return value; } bool has_alias() const { return alias; } @@ -142,6 +156,7 @@ class TestFactory7 { class PyTF7 : public TestFactory7 { public: explicit PyTF7(int i) : TestFactory7(i) { + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) alias = true; print_created(this, i); } diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index a5b746cad4..1790c6caca 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -152,23 +152,26 @@ TEST_SUBMODULE(sequences_and_iterators, m) { public: explicit Sequence(size_t size) : m_size(size) { print_created(this, "of size", m_size); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) m_data = new float[size]; memset(m_data, 0, sizeof(float) * size); } explicit Sequence(const std::vector &value) : m_size(value.size()) { print_created(this, "of size", m_size, "from std::vector"); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) m_data = new float[m_size]; memcpy(m_data, &value[0], sizeof(float) * m_size); } Sequence(const Sequence &s) : m_size(s.m_size) { print_copy_created(this); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) m_data = new float[m_size]; memcpy(m_data, s.m_data, sizeof(float)*m_size); } Sequence(Sequence &&s) noexcept : m_size(s.m_size), m_data(s.m_data) { print_move_created(this); + // NOTLINENEXTLINE(cppcoreguidelines-prefer-member-initializer) s.m_size = 0; - s.m_data = nullptr; } ~Sequence() { print_destroyed(this); delete[] m_data; } @@ -236,7 +239,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) { private: size_t m_size; - float *m_data; + float *m_data = nullptr; }; py::class_(m, "Sequence") .def(py::init()) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index c86bed1ce0..b2b91a62db 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -131,8 +131,7 @@ class MyObject4a; std::unordered_set myobject4a_instances; class MyObject4a { public: - explicit MyObject4a(int i) { - value = i; + explicit MyObject4a(int i) : value{i} { print_created(this); myobject4a_instances.insert(this); }; diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 7fb90cc591..558cd94406 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -103,10 +103,7 @@ class PyExampleVirt : public ExampleVirt { class NonCopyable { public: NonCopyable(int a, int b) : value{new int(a*b)} { print_created(this, a, b); } - NonCopyable(NonCopyable &&o) noexcept { - value = std::move(o.value); - print_move_created(this); - } + NonCopyable(NonCopyable &&o) noexcept : value{std::move(o.value)} { print_move_created(this); } NonCopyable(const NonCopyable &) = delete; NonCopyable() = delete; void operator=(const NonCopyable &) = delete; @@ -128,11 +125,8 @@ class NonCopyable { class Movable { public: Movable(int a, int b) : value{a+b} { print_created(this, a, b); } - Movable(const Movable &m) { value = m.value; print_copy_created(this); } - Movable(Movable &&m) noexcept { - value = m.value; - print_move_created(this); - } + Movable(const Movable &m) : value{m.value} { print_copy_created(this); } + Movable(Movable &&m) noexcept : value{m.value} { print_move_created(this); } std::string get_value() const { return std::to_string(value); } ~Movable() { print_destroyed(this); } private: From 2b8664dd02d0ce6b41dd2bf66f6a25d1662b9c1c Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 10 Feb 2022 11:52:00 -0500 Subject: [PATCH 2/4] Fix clang-tdy config --- .clang-tidy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.clang-tidy b/.clang-tidy index 3d80f4fe72..3c4292010f 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -3,6 +3,7 @@ FormatStyle: file Checks: ' *bugprone*, cppcoreguidelines-init-variables, +cppcoreguidelines-prefer-member-initializer, cppcoreguidelines-pro-type-static-cast-downcast, cppcoreguidelines-slicing, clang-analyzer-optin.performance.Padding, @@ -66,6 +67,6 @@ CheckOptions: - key: readability-implicit-bool-conversion.AllowPointerConditions value: true -HeaderFilterRegex: '(^(include/pybind11/.*h | tests/*.h))' +HeaderFilterRegex: 'pybind11/.*h' WarningsAsErrors: '*' From 12fe520925f027e912d5ad9aba9dcf7b9200d6dd Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 10 Feb 2022 11:57:35 -0500 Subject: [PATCH 3/4] Fix incorrect change --- tests/test_sequences_and_iterators.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index 1790c6caca..5d4275f36d 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -170,8 +170,8 @@ TEST_SUBMODULE(sequences_and_iterators, m) { } Sequence(Sequence &&s) noexcept : m_size(s.m_size), m_data(s.m_data) { print_move_created(this); - // NOTLINENEXTLINE(cppcoreguidelines-prefer-member-initializer) s.m_size = 0; + s.m_data = nullptr; } ~Sequence() { print_destroyed(this); delete[] m_data; } @@ -239,7 +239,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) { private: size_t m_size; - float *m_data = nullptr; + float *m_data; }; py::class_(m, "Sequence") .def(py::init()) From 8d62d5a87b1030f96dffe00ced7701dec50d7a3d Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 10 Feb 2022 12:08:27 -0500 Subject: [PATCH 4/4] Fix sorting of .clang-tidy --- .clang-tidy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 3c4292010f..d01ca352cc 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -2,12 +2,12 @@ FormatStyle: file Checks: ' *bugprone*, +clang-analyzer-optin.performance.Padding, +clang-analyzer-optin.cplusplus.VirtualCall, cppcoreguidelines-init-variables, cppcoreguidelines-prefer-member-initializer, cppcoreguidelines-pro-type-static-cast-downcast, cppcoreguidelines-slicing, -clang-analyzer-optin.performance.Padding, -clang-analyzer-optin.cplusplus.VirtualCall, google-explicit-constructor, llvm-namespace-comment, misc-misplaced-const,