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

@@ -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,

include/pybind11/cast.h

@@ -86,24 +86,27 @@ public:
 };
 
 #define PYBIND11_TYPE_CASTER(type, py_name)                                                       \
-    protected: \
+protected:                                                                                        \
     type value;                                                                                   \
-    public: \
+                                                                                                  \
+public:                                                                                           \
     static constexpr auto name = py_name;                                                         \
     template <typename T_, enable_if_t<std::is_same<type, remove_cv_t<T_>>::value, int> = 0>      \
     static handle cast(T_ *src, return_value_policy policy, handle parent) {                      \
-            if (!src) return none().release(); \
+        if (!src)                                                                                 \
+            return none().release();                                                              \
         if (policy == return_value_policy::take_ownership) {                                      \
-                auto h = cast(std::move(*src), policy, parent); delete src; return h; \
+            auto h = cast(std::move(*src), policy, parent);                                       \
-            } else { \
+            delete src;                                                                           \
+            return h;                                                                             \
+        }                                                                                         \
         return cast(*src, policy, parent);                                                        \
     }                                                                                             \
-        } \
+    operator type *() { return &value; }                                                          \
-        operator type*() { return &value; } \
+    operator type &() { return value; }                                                           \
-        operator type&() { return value; } \
+    operator type &&() && { return std::move(value); }                                            \
-        operator type&&() && { return std::move(value); } \
+    template <typename T_>                                                                        \
-        template <typename T_> using cast_op_type = pybind11::detail::movable_cast_op_type<T_>
+    using cast_op_type = pybind11::detail::movable_cast_op_type<T_>
-
 
 template <typename CharT> using is_std_char_type = any_of<
     std::is_same<CharT, char>, /* 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,7 +276,6 @@ public:
     static handle cast(const void *ptr, return_value_policy /* policy */, handle /* parent */) {
         if (ptr)
             return capsule(ptr).release();
-        else
         return none().inc_ref();
     }
 
@@ -289,9 +292,15 @@ template <> class type_caster<bool> {
 public:
     bool load(handle src, bool convert) {
         if (!src) return false;
-        else if (src.ptr() == Py_True) { value = true; return true; }
+        if (src.ptr() == Py_True) {
-        else if (src.ptr() == Py_False) { value = false; return true; }
+            value = true;
-        else if (convert || !std::strcmp("numpy.bool_", Py_TYPE(src.ptr())->tp_name)) {
+            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 <typename StringType, bool IsView = false> 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;
+            auto h = cast(std::move(*src), policy, parent);
-        } else {
+            delete src;
-            return cast(*src, policy, parent);
+            return h;
         }
+        return cast(*src, policy, parent);
     }
 
     static constexpr auto name = _("Tuple[") + concat(make_caster<Ts>::name...) + _("]");
@@ -664,15 +674,15 @@ protected:
             value = v_h.value_ptr();
             holder = v_h.template holder<holder_type>();
             return true;
-        } else {
+        }
         throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "
 #if defined(NDEBUG)
                          "(compile in debug mode for type information)");
 #else
-                             "of type '" + type_id<holder_type>() + "''");
+                         "of type '"
+                         + type_id<holder_type>() + "''");
 #endif
     }
-    }
 
     template <typename T = holder_type, detail::enable_if_t<!std::is_constructible<T, const T &, type*>::value, int> = 0>
     bool try_implicit_casts(handle, bool) { return false; }
@@ -917,7 +927,6 @@ template <typename T> detail::enable_if_t<detail::move_always<T>::value, T> cast
 template <typename T> detail::enable_if_t<detail::move_if_unreferenced<T>::value, T> cast(object &&object) {
     if (object.ref_count() > 1)
         return cast<T>(object);
-    else
     return move<T>(std::move(object));
 }
 template <typename T> detail::enable_if_t<detail::move_never<T>::value, T> cast(object &&object) {

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<rep, period>>(duration<double>(PyFloat_AsDouble(src.ptr()))));
             return true;
         }
-        else return false;
+        return false;
     }
 
     // If this is a duration just return it back

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);
-    }
 }
 #endif
 

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<instance *>(src.ptr())->get_value_and_holder(base));

include/pybind11/eigen.h

@@ -169,22 +169,19 @@ template <typename Type_> 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};
-        }
+        } // Otherwise it's either fully dynamic, or column dynamic; both become a column vector
-        else {
-            // 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<Type>::value && is_eigen_mutable_map<Type>::value;
     static constexpr bool show_order = is_eigen_dense_map<Type>::value;

include/pybind11/functional.h

@@ -98,7 +98,6 @@ public:
         auto result = f_.template target<function_type>();
         if (result)
             return cpp_function(*result, policy).release();
-        else
         return cpp_function(std::forward<Func>(f_), policy).release();
     }
 

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,7 +1492,6 @@ struct vectorize_returned_array {
     static Type create(broadcast_trivial trivial, const std::vector<ssize_t> &shape) {
         if (trivial == broadcast_trivial::f_trivial)
             return array_t<Return, array::f_style>(shape);
-        else
         return array_t<Return>(shape);
     }
 

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<PyCFunction>(reinterpret_cast<void (*)(void)>(dispatcher));
+                = reinterpret_cast<PyCFunction>(reinterpret_cast<void (*)()>(dispatcher));
             rec->def->ml_flags = METH_VARARGS | METH_KEYWORDS;
 
             capsule rec_capsule(unique_rec.release(), [](void *ptr) {
@@ -924,21 +924,21 @@ 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<instance *>(parent.ptr());
             self_value_and_holder.type->init_instance(pi, nullptr);
         }
         return result.ptr();
     }
-    }
 };
 
 /// Wrapper for Python extension modules
@@ -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();
     };
@@ -2189,14 +2189,15 @@ template <class T> function get_override(const T *this_ptr, const char *name) {
 #define PYBIND11_OVERRIDE_IMPL(ret_type, cname, name, ...)                                        \
     do {                                                                                          \
         pybind11::gil_scoped_acquire gil;                                                         \
-        pybind11::function override = pybind11::get_override(static_cast<const cname *>(this), name); \
+        pybind11::function override                                                               \
+            = pybind11::get_override(static_cast<const cname *>(this), name);                     \
         if (override) {                                                                           \
             auto o = override(__VA_ARGS__);                                                       \
             if (pybind11::detail::cast_is_temporary_value_reference<ret_type>::value) {           \
                 static pybind11::detail::override_caster_t<ret_type> caster;                      \
                 return pybind11::detail::cast_ref<ret_type>(std::move(o), caster);                \
             }                                                                                     \
-            else return pybind11::detail::cast_safe<ret_type>(std::move(o)); \
+            return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \
         }                                                                                         \
     } while (false)
 

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<object>(result);
-    } else {
+    }
     PyErr_Clear();
     return reinterpret_borrow<object>(default_);
-    }
 }
 
 inline object getattr(handle obj, const char *name, handle default_) {
     if (PyObject *result = PyObject_GetAttrString(obj.ptr(), name)) {
         return reinterpret_steal<object>(result);
-    } else {
+    }
     PyErr_Clear();
     return reinterpret_borrow<object>(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;
-    }
 }
 
 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;
-    }
 }
 PYBIND11_NAMESPACE_END(detail)
 

include/pybind11/stl.h

@@ -278,7 +278,8 @@ template<typename T> 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;

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;
-        }
+
     }));
 
     return;

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<Payload>);
     /* Test cleanup of lambda closure */
-    m.def("test_cleanup", []() -> std::function<void(void)> {
+    m.def("test_cleanup", []() -> std::function<void()> {
         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 {

tests/test_class.cpp

@@ -154,7 +154,6 @@ TEST_SUBMODULE(class_, m) {
         //     return py::type::of<int>();
         if (category == 1)
             return py::type::of<DerivedClass1>();
-        else
         return py::type::of<Invalid>();
     });
 

tests/test_copy_move.cpp

@@ -203,7 +203,6 @@ TEST_SUBMODULE(copy_move_policies, m) {
             void *ptr = std::malloc(bytes);
             if (ptr)
                 return ptr;
-            else
             throw std::bad_alloc{};
         }
     };

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", []() {

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); }