From b5357d1fa8e91ddbfbc2ad057b9a1ccb74becbba Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 9 Jul 2021 09:45:53 -0400 Subject: [PATCH] fix(clang-tidy): Enable clang-tidy else-after-return and redundant void checks (#3080) * Enable clang-tidy else-after-return and redundant void checks * Fix remaining else-after * Address reviewer comments * Fix indentation * Rerun clang-tidy post merge --- .clang-tidy | 3 + include/pybind11/cast.h | 89 ++++++++++++---------- include/pybind11/chrono.h | 4 +- include/pybind11/detail/class.h | 4 +- include/pybind11/detail/type_caster_base.h | 4 +- include/pybind11/eigen.h | 9 +-- include/pybind11/functional.h | 3 +- include/pybind11/numpy.h | 6 +- include/pybind11/pybind11.h | 45 +++++------ include/pybind11/pytypes.h | 21 ++--- include/pybind11/stl.h | 3 +- include/pybind11/stl_bind.h | 13 ++-- tests/test_callbacks.cpp | 9 ++- tests/test_class.cpp | 3 +- tests/test_copy_move.cpp | 3 +- tests/test_pytypes.cpp | 10 +-- tests/test_virtual_functions.cpp | 4 +- 17 files changed, 116 insertions(+), 117 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 22a736208..ea50bf39b 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,11 +1,13 @@ FormatStyle: file Checks: ' +clang-analyzer-optin.cplusplus.VirtualCall, llvm-namespace-comment, misc-misplaced-const, misc-static-assert, misc-uniqueptr-reset-release, modernize-avoid-bind, +modernize-redundant-void-arg, modernize-replace-auto-ptr, modernize-replace-disallow-copy-and-assign-macro, modernize-shrink-to-fit, @@ -20,6 +22,7 @@ modernize-use-override, modernize-use-using, *performance*, readability-container-size-empty, +readability-else-after-return, readability-make-member-function-const, readability-redundant-function-ptr-dereference, readability-redundant-smartptr-get, diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 4a219e2df..a748c77c0 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -85,25 +85,28 @@ public: operator std::reference_wrapper() { return cast_op(subcaster); } }; -#define PYBIND11_TYPE_CASTER(type, py_name) \ - protected: \ - type value; \ - public: \ - static constexpr auto name = py_name; \ - template >::value, int> = 0> \ - static handle cast(T_ *src, return_value_policy policy, handle parent) { \ - if (!src) return none().release(); \ - if (policy == return_value_policy::take_ownership) { \ - auto h = cast(std::move(*src), policy, parent); delete src; return h; \ - } else { \ - return cast(*src, policy, parent); \ - } \ - } \ - operator type*() { return &value; } \ - operator type&() { return value; } \ - operator type&&() && { return std::move(value); } \ - template using cast_op_type = pybind11::detail::movable_cast_op_type - +#define PYBIND11_TYPE_CASTER(type, py_name) \ +protected: \ + type value; \ + \ +public: \ + static constexpr auto name = py_name; \ + template >::value, int> = 0> \ + static handle cast(T_ *src, return_value_policy policy, handle parent) { \ + if (!src) \ + return none().release(); \ + if (policy == return_value_policy::take_ownership) { \ + auto h = cast(std::move(*src), policy, parent); \ + delete src; \ + return h; \ + } \ + return cast(*src, policy, parent); \ + } \ + operator type *() { return &value; } \ + operator type &() { return value; } \ + operator type &&() && { return std::move(value); } \ + template \ + using cast_op_type = pybind11::detail::movable_cast_op_type template using is_std_char_type = any_of< std::is_same, /* std::string */ @@ -247,7 +250,8 @@ public: bool load(handle h, bool) { if (!h) { return false; - } else if (h.is_none()) { + } + if (h.is_none()) { value = nullptr; return true; } @@ -272,8 +276,7 @@ public: static handle cast(const void *ptr, return_value_policy /* policy */, handle /* parent */) { if (ptr) return capsule(ptr).release(); - else - return none().inc_ref(); + return none().inc_ref(); } template using cast_op_type = void*&; @@ -289,9 +292,15 @@ template <> class type_caster { public: bool load(handle src, bool convert) { if (!src) return false; - else if (src.ptr() == Py_True) { value = true; return true; } - else if (src.ptr() == Py_False) { value = false; return true; } - else if (convert || !std::strcmp("numpy.bool_", Py_TYPE(src.ptr())->tp_name)) { + if (src.ptr() == Py_True) { + value = true; + return true; + } + if (src.ptr() == Py_False) { + value = false; + return true; + } + if (convert || !std::strcmp("numpy.bool_", Py_TYPE(src.ptr())->tp_name)) { // (allow non-implicit conversion for numpy booleans) Py_ssize_t res = -1; @@ -315,9 +324,8 @@ public: if (res == 0 || res == 1) { value = (bool) res; return true; - } else { - PyErr_Clear(); } + PyErr_Clear(); } return false; } @@ -351,7 +359,8 @@ template struct string_caster { handle load_src = src; if (!src) { return false; - } else if (!PyUnicode_Check(load_src.ptr())) { + } + if (!PyUnicode_Check(load_src.ptr())) { #if PY_MAJOR_VERSION >= 3 return load_bytes(load_src); #else @@ -554,10 +563,11 @@ public: static handle cast(T *src, return_value_policy policy, handle parent) { if (!src) return none().release(); if (policy == return_value_policy::take_ownership) { - auto h = cast(std::move(*src), policy, parent); delete src; return h; - } else { - return cast(*src, policy, parent); + auto h = cast(std::move(*src), policy, parent); + delete src; + return h; } + return cast(*src, policy, parent); } static constexpr auto name = _("Tuple[") + concat(make_caster::name...) + _("]"); @@ -664,14 +674,14 @@ protected: value = v_h.value_ptr(); holder = v_h.template holder(); return true; - } else { - throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " -#if defined(NDEBUG) - "(compile in debug mode for type information)"); -#else - "of type '" + type_id() + "''"); -#endif } + throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " +#if defined(NDEBUG) + "(compile in debug mode for type information)"); +#else + "of type '" + + type_id() + "''"); +#endif } template ::value, int> = 0> @@ -917,8 +927,7 @@ template detail::enable_if_t::value, T> cast template detail::enable_if_t::value, T> cast(object &&object) { if (object.ref_count() > 1) return cast(object); - else - return move(std::move(object)); + return move(std::move(object)); } template detail::enable_if_t::value, T> cast(object &&object) { return cast(object); diff --git a/include/pybind11/chrono.h b/include/pybind11/chrono.h index 317afd183..d32b5b056 100644 --- a/include/pybind11/chrono.h +++ b/include/pybind11/chrono.h @@ -53,11 +53,11 @@ public: return true; } // If invoked with a float we assume it is seconds and convert - else if (PyFloat_Check(src.ptr())) { + if (PyFloat_Check(src.ptr())) { value = type(duration_cast>(duration(PyFloat_AsDouble(src.ptr())))); return true; } - else return false; + return false; } // If this is a duration just return it back diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index ca6854e7b..5fee3318b 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -162,9 +162,7 @@ extern "C" inline PyObject *pybind11_meta_getattro(PyObject *obj, PyObject *name Py_INCREF(descr); return descr; } - else { - return PyType_Type.tp_getattro(obj, name); - } + return PyType_Type.tp_getattro(obj, name); } #endif diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a8d3938c7..451690dc7 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -670,7 +670,7 @@ public: return true; } // Case 2: We have a derived class - else if (PyType_IsSubtype(srctype, typeinfo->type)) { + if (PyType_IsSubtype(srctype, typeinfo->type)) { auto &bases = all_type_info(srctype); bool no_cpp_mi = typeinfo->simple_type; @@ -687,7 +687,7 @@ public: // Case 2b: the python type inherits from multiple C++ bases. Check the bases to see if // we can find an exact match (or, for a simple C++ type, an inherited match); if so, we // can safely reinterpret_cast to the relevant pointer. - else if (bases.size() > 1) { + if (bases.size() > 1) { for (auto base : bases) { if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(base)); diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index e8c6f6339..218fe2703 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -169,21 +169,18 @@ template struct EigenProps { return false; // Vector size mismatch return {rows == 1 ? 1 : n, cols == 1 ? 1 : n, stride}; } - else if (fixed) { + if (fixed) { // The type has a fixed size, but is not a vector: abort return false; } - else if (fixed_cols) { + if (fixed_cols) { // Since this isn't a vector, cols must be != 1. We allow this only if it exactly // equals the number of elements (rows is Dynamic, and so 1 row is allowed). if (cols != n) return false; return {1, n, stride}; - } - else { - // Otherwise it's either fully dynamic, or column dynamic; both become a column vector + } // Otherwise it's either fully dynamic, or column dynamic; both become a column vector if (fixed_rows && rows != n) return false; return {n, 1, stride}; - } } static constexpr bool show_writeable = is_eigen_dense_map::value && is_eigen_mutable_map::value; diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index aee9be4e4..cc7099c6a 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -98,8 +98,7 @@ public: auto result = f_.template target(); if (result) return cpp_function(*result, policy).release(); - else - return cpp_function(std::forward(f_), policy).release(); + return cpp_function(std::forward(f_), policy).release(); } PYBIND11_TYPE_CASTER(type, _("Callable[[") + concat(make_caster::name...) + _("], ") diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index fdb7a9794..edba8bac9 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -1340,9 +1340,8 @@ public: if (++m_index[i] != m_shape[i]) { increment_common_iterator(i); break; - } else { - m_index[i] = 0; } + m_index[i] = 0; } return *this; } @@ -1493,8 +1492,7 @@ struct vectorize_returned_array { static Type create(broadcast_trivial trivial, const std::vector &shape) { if (trivial == broadcast_trivial::f_trivial) return array_t(shape); - else - return array_t(shape); + return array_t(shape); } static Return *mutable_data(Type &array) { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4d7bc5c73..616fa7025 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -396,7 +396,7 @@ protected: std::memset(rec->def, 0, sizeof(PyMethodDef)); rec->def->ml_name = rec->name; rec->def->ml_meth - = reinterpret_cast(reinterpret_cast(dispatcher)); + = reinterpret_cast(reinterpret_cast(dispatcher)); rec->def->ml_flags = METH_VARARGS | METH_KEYWORDS; capsule rec_capsule(unique_rec.release(), [](void *ptr) { @@ -924,20 +924,20 @@ protected: append_note_if_missing_header_is_suspected(msg); PyErr_SetString(PyExc_TypeError, msg.c_str()); return nullptr; - } else if (!result) { + } + if (!result) { std::string msg = "Unable to convert function return value to a " "Python type! The signature was\n\t"; msg += it->signature; append_note_if_missing_header_is_suspected(msg); PyErr_SetString(PyExc_TypeError, msg.c_str()); return nullptr; - } else { - if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) { - auto *pi = reinterpret_cast(parent.ptr()); - self_value_and_holder.type->init_instance(pi, nullptr); - } - return result.ptr(); } + if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) { + auto *pi = reinterpret_cast(parent.ptr()); + self_value_and_holder.type->init_instance(pi, nullptr); + } + return result.ptr(); } }; @@ -1860,9 +1860,9 @@ PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, func auto get_arg = [&](size_t n) { if (n == 0) return ret; - else if (n == 1 && call.init_self) + if (n == 1 && call.init_self) return call.init_self; - else if (n <= call.args.size()) + if (n <= call.args.size()) return call.args[n - 1]; return handle(); }; @@ -2186,18 +2186,19 @@ template function get_override(const T *this_ptr, const char *name) { return tinfo ? detail::get_type_override(this_ptr, tinfo, name) : function(); } -#define PYBIND11_OVERRIDE_IMPL(ret_type, cname, name, ...) \ - do { \ - pybind11::gil_scoped_acquire gil; \ - pybind11::function override = pybind11::get_override(static_cast(this), name); \ - if (override) { \ - auto o = override(__VA_ARGS__); \ - if (pybind11::detail::cast_is_temporary_value_reference::value) { \ - static pybind11::detail::override_caster_t caster; \ - return pybind11::detail::cast_ref(std::move(o), caster); \ - } \ - else return pybind11::detail::cast_safe(std::move(o)); \ - } \ +#define PYBIND11_OVERRIDE_IMPL(ret_type, cname, name, ...) \ + do { \ + pybind11::gil_scoped_acquire gil; \ + pybind11::function override \ + = pybind11::get_override(static_cast(this), name); \ + if (override) { \ + auto o = override(__VA_ARGS__); \ + if (pybind11::detail::cast_is_temporary_value_reference::value) { \ + static pybind11::detail::override_caster_t caster; \ + return pybind11::detail::cast_ref(std::move(o), caster); \ + } \ + return pybind11::detail::cast_safe(std::move(o)); \ + } \ } while (false) /** \rst diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 80637fb98..fb9680dfa 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -440,19 +440,17 @@ inline object getattr(handle obj, const char *name) { inline object getattr(handle obj, handle name, handle default_) { if (PyObject *result = PyObject_GetAttr(obj.ptr(), name.ptr())) { return reinterpret_steal(result); - } else { - PyErr_Clear(); - return reinterpret_borrow(default_); } + PyErr_Clear(); + return reinterpret_borrow(default_); } inline object getattr(handle obj, const char *name, handle default_) { if (PyObject *result = PyObject_GetAttrString(obj.ptr(), name)) { return reinterpret_steal(result); - } else { - PyErr_Clear(); - return reinterpret_borrow(default_); } + PyErr_Clear(); + return reinterpret_borrow(default_); } inline void setattr(handle obj, handle name, handle value) { @@ -791,10 +789,9 @@ inline bool PyIterable_Check(PyObject *obj) { if (iter) { Py_DECREF(iter); return true; - } else { - PyErr_Clear(); - return false; } + PyErr_Clear(); + return false; } inline bool PyNone_Check(PyObject *o) { return o == Py_None; } @@ -1188,10 +1185,8 @@ Unsigned as_unsigned(PyObject *o) { unsigned long v = PyLong_AsUnsignedLong(o); return v == (unsigned long) -1 && PyErr_Occurred() ? (Unsigned) -1 : (Unsigned) v; } - else { - unsigned long long v = PyLong_AsUnsignedLongLong(o); - return v == (unsigned long long) -1 && PyErr_Occurred() ? (Unsigned) -1 : (Unsigned) v; - } + unsigned long long v = PyLong_AsUnsignedLongLong(o); + return v == (unsigned long long) -1 && PyErr_Occurred() ? (Unsigned) -1 : (Unsigned) v; } PYBIND11_NAMESPACE_END(detail) diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index ca20b7483..51994c656 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -278,7 +278,8 @@ template struct optional_caster { bool load(handle src, bool convert) { if (!src) { return false; - } else if (src.is_none()) { + } + if (src.is_none()) { return true; // default-constructed value is already empty } value_conv inner_caster; diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index b78bd27ce..76700763f 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -414,13 +414,12 @@ void vector_buffer_impl(Class_& cl, std::true_type) { if (step == 1) { return Vector(p, end); } - else { - Vector vec; - vec.reserve((size_t) info.shape[0]); - for (; p != end; p += step) - vec.push_back(*p); - return vec; - } + Vector vec; + vec.reserve((size_t) info.shape[0]); + for (; p != end; p += step) + vec.push_back(*p); + return vec; + })); return; diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index 908d5743e..7e79217a3 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -82,7 +82,7 @@ TEST_SUBMODULE(callbacks, m) { // Export the payload constructor statistics for testing purposes: m.def("payload_cstats", &ConstructorStats::get); /* Test cleanup of lambda closure */ - m.def("test_cleanup", []() -> std::function { + m.def("test_cleanup", []() -> std::function { Payload p; return [p]() { @@ -108,12 +108,13 @@ TEST_SUBMODULE(callbacks, m) { if (!result) { auto r = f(1); return "can't convert to function pointer: eval(1) = " + std::to_string(r); - } else if (*result == dummy_function) { + } + if (*result == dummy_function) { auto r = (*result)(1); return "matches dummy_function: eval(1) = " + std::to_string(r); - } else { - return "argument does NOT match dummy_function. This should never happen!"; } + return "argument does NOT match dummy_function. This should never happen!"; + }); class AbstractBase { diff --git a/tests/test_class.cpp b/tests/test_class.cpp index a26c59c1c..bcc73ef0b 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -154,8 +154,7 @@ TEST_SUBMODULE(class_, m) { // return py::type::of(); if (category == 1) return py::type::of(); - else - return py::type::of(); + return py::type::of(); }); m.def("get_type_of", [](py::object ob) { return py::type::of(std::move(ob)); }); diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 08f4dd749..05caf28ff 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -203,8 +203,7 @@ TEST_SUBMODULE(copy_move_policies, m) { void *ptr = std::malloc(bytes); if (ptr) return ptr; - else - throw std::bad_alloc{}; + throw std::bad_alloc{}; } }; py::class_(m, "PrivateOpNew").def_readonly("value", &PrivateOpNew::value); diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 9e0ebc206..2b91af37b 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -263,13 +263,13 @@ TEST_SUBMODULE(pytypes, m) { if (type == "bytes") { return move ? py::bytes(std::move(value)) : py::bytes(value); } - else if (type == "none") { + if (type == "none") { return move ? py::none(std::move(value)) : py::none(value); } - else if (type == "ellipsis") { + if (type == "ellipsis") { return move ? py::ellipsis(std::move(value)) : py::ellipsis(value); } - else if (type == "type") { + if (type == "type") { return move ? py::type(std::move(value)) : py::type(value); } throw std::runtime_error("Invalid type"); @@ -385,9 +385,7 @@ TEST_SUBMODULE(pytypes, m) { if (is_unsigned) return py::memoryview::from_buffer( ui16, { 4 }, { sizeof(uint16_t) }); - else - return py::memoryview::from_buffer( - si16, { 5 }, { sizeof(int16_t) }); + return py::memoryview::from_buffer(si16, {5}, {sizeof(int16_t)}); }); m.def("test_memoryview_from_buffer_nativeformat", []() { diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 51068f638..5280af8eb 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -112,7 +112,9 @@ public: void operator=(const NonCopyable &) = delete; void operator=(NonCopyable &&) = delete; std::string get_value() const { - if (value) return std::to_string(*value); else return "(null)"; + if (value) + return std::to_string(*value); + return "(null)"; } ~NonCopyable() { print_destroyed(this); }