maint(clang-tidy): Improve code readability with explicit boolean casts (#3148)

* maint(clang-tidy) Improve code readability

* Fix minor typos

* Revert optimization that removed test case

* Fix comment formatting

* Revert another optimization to repro an issue

* Remove make_unique since it C++14 and newer only

* eformat comments

* Fix unsignedness of comparison

* Update comment

Author: Skylion007

.clang-tidy

@@ -5,13 +5,17 @@ cppcoreguidelines-init-variables,
 clang-analyzer-optin.cplusplus.VirtualCall,
 llvm-namespace-comment,
 misc-misplaced-const,
+misc-non-copyable-objects,
 misc-static-assert,
 misc-throw-by-value-catch-by-reference,
 misc-uniqueptr-reset-release,
+misc-unused-parameters,
 modernize-avoid-bind,
+modernize-make-shared,
 modernize-redundant-void-arg,
 modernize-replace-auto-ptr,
 modernize-replace-disallow-copy-and-assign-macro,
+modernize-replace-random-shuffle,
 modernize-shrink-to-fit,
 modernize-use-auto,
 modernize-use-bool-literals,
@@ -23,13 +27,20 @@ modernize-use-emplace,
 modernize-use-override,
 modernize-use-using,
 *performance*,
+readability-avoid-const-params-in-decls,
 readability-container-size-empty,
 readability-else-after-return,
+readability-delete-null-pointer,
+readability-implicit-bool-conversion,
 readability-make-member-function-const,
+readability-misplaced-array-index,
+readability-non-const-parameter,
 readability-redundant-function-ptr-dereference,
 readability-redundant-smartptr-get,
 readability-redundant-string-cstr,
 readability-simplify-subscript-expr,
+readability-static-accessed-through-instance,
+readability-static-definition-in-anonymous-namespace,
 readability-string-compare,
 readability-uniqueptr-delete-release,
 '
@@ -39,6 +50,8 @@ CheckOptions:
   value:           true
 - key:             performance-unnecessary-value-param.AllowedTypes
   value:           'exception_ptr$;'
+- key:             readability-implicit-bool-conversion.AllowPointerConditions
+  value:           true
 
 HeaderFilterRegex: 'pybind11/.*h'
 

include/pybind11/cast.h

@@ -181,7 +181,7 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t
         // Signed/unsigned checks happen elsewhere
         if (py_err || (std::is_integral<T>::value && sizeof(py_type) != sizeof(T) && py_value != (py_type) (T) py_value)) {
             PyErr_Clear();
-            if (py_err && convert && PyNumber_Check(src.ptr())) {
+            if (py_err && convert && (PyNumber_Check(src.ptr()) != 0)) {
                 auto tmp = reinterpret_steal<object>(std::is_floating_point<T>::value
                                                      ? PyNumber_Float(src.ptr())
                                                      : PyNumber_Long(src.ptr()));
@@ -300,7 +300,7 @@ template <> class type_caster<bool> {
             value = false;
             return true;
         }
-        if (convert || !std::strcmp("numpy.bool_", Py_TYPE(src.ptr())->tp_name)) {
+        if (convert || (std::strcmp("numpy.bool_", Py_TYPE(src.ptr())->tp_name) == 0)) {
             // (allow non-implicit conversion for numpy booleans)
 
             Py_ssize_t res = -1;
@@ -501,10 +501,14 @@ template <typename CharT> struct type_caster<CharT, enable_if_t<is_std_char_type
         // can fit into a single char value.
         if (StringCaster::UTF_N == 8 && str_len > 1 && str_len <= 4) {
             auto v0 = static_cast<unsigned char>(value[0]);
-            size_t char0_bytes = !(v0 & 0x80) ? 1 : // low bits only: 0-127
-                (v0 & 0xE0) == 0xC0 ? 2 : // 0b110xxxxx - start of 2-byte sequence
-                (v0 & 0xF0) == 0xE0 ? 3 : // 0b1110xxxx - start of 3-byte sequence
-                4; // 0b11110xxx - start of 4-byte sequence
+            // low bits only: 0-127
+            // 0b110xxxxx - start of 2-byte sequence
+            // 0b1110xxxx - start of 3-byte sequence
+            // 0b11110xxx - start of 4-byte sequence
+            size_t char0_bytes = (v0 & 0x80) == 0      ? 1
+                                 : (v0 & 0xE0) == 0xC0 ? 2
+                                 : (v0 & 0xF0) == 0xE0 ? 3
+                                                       : 4;
 
             if (char0_bytes == str_len) {
                 // If we have a 128-255 value, we can decode it into a single char:

include/pybind11/detail/class.h

@@ -129,8 +129,9 @@ extern "C" inline int pybind11_meta_setattro(PyObject* obj, PyObject* name, PyOb
     //   2. `Type.static_prop = other_static_prop` --> setattro:  replace existing `static_prop`
     //   3. `Type.regular_attribute = value`       --> setattro:  regular attribute assignment
     const auto static_prop = (PyObject *) get_internals().static_property_type;
-    const auto call_descr_set = descr && value && PyObject_IsInstance(descr, static_prop)
-                                && !PyObject_IsInstance(value, static_prop);
+    const auto call_descr_set = (descr != nullptr) && (value != nullptr)
+                                && (PyObject_IsInstance(descr, static_prop) != 0)
+                                && (PyObject_IsInstance(value, static_prop) == 0);
     if (call_descr_set) {
         // Call `static_property.__set__()` instead of replacing the `static_property`.
 #if !defined(PYPY_VERSION)
@@ -562,7 +563,7 @@ extern "C" inline int pybind11_getbuffer(PyObject *obj, Py_buffer *view, int fla
     view->len = view->itemsize;
     for (auto s : info->shape)
         view->len *= s;
-    view->readonly = info->readonly;
+    view->readonly = static_cast<int>(info->readonly);
     if ((flags & PyBUF_FORMAT) == PyBUF_FORMAT)
         view->format = const_cast<char *>(info->format.c_str());
     if ((flags & PyBUF_STRIDES) == PyBUF_STRIDES) {

include/pybind11/detail/internals.h

@@ -297,7 +297,7 @@ PYBIND11_NOINLINE inline internals &get_internals() {
         PyThreadState *tstate = PyThreadState_Get();
         #if PY_VERSION_HEX >= 0x03070000
             internals_ptr->tstate = PyThread_tss_alloc();
-            if (!internals_ptr->tstate || PyThread_tss_create(internals_ptr->tstate))
+            if (!internals_ptr->tstate || (PyThread_tss_create(internals_ptr->tstate) != 0))
                 pybind11_fail("get_internals: could not successfully initialize the TSS key!");
             PyThread_tss_set(internals_ptr->tstate, tstate);
         #else

include/pybind11/detail/type_caster_base.h

@@ -238,8 +238,8 @@ struct value_and_holder {
     }
     bool holder_constructed() const {
         return inst->simple_layout
-            ? inst->simple_holder_constructed
-            : inst->nonsimple.status[index] & instance::status_holder_constructed;
+                   ? inst->simple_holder_constructed
+                   : (inst->nonsimple.status[index] & instance::status_holder_constructed) != 0u;
     }
     void set_holder_constructed(bool v = true) const {
         if (inst->simple_layout)

include/pybind11/embed.h

@@ -76,7 +76,7 @@ struct embedded_module {
     using init_t = void (*)();
 #endif
     embedded_module(const char *name, init_t init) {
-        if (Py_IsInitialized())
+        if (Py_IsInitialized() != 0)
             pybind11_fail("Can't add new modules after the interpreter has been initialized");
 
         auto result = PyImport_AppendInittab(name, init);
@@ -101,7 +101,7 @@ PYBIND11_NAMESPACE_END(detail)
     .. _Python documentation: https://docs.python.org/3/c-api/init.html#c.Py_InitializeEx
  \endrst */
 inline void initialize_interpreter(bool init_signal_handlers = true) {
-    if (Py_IsInitialized())
+    if (Py_IsInitialized() != 0)
         pybind11_fail("The interpreter is already running");
 
     Py_InitializeEx(init_signal_handlers ? 1 : 0);

include/pybind11/numpy.h

@@ -465,7 +465,9 @@ class dtype : public object {
     explicit dtype(const buffer_info &info) {
         dtype descr(_dtype_from_pep3118()(PYBIND11_STR_TYPE(info.format)));
         // If info.itemsize == 0, use the value calculated from the format string
-        m_ptr = descr.strip_padding(info.itemsize ? info.itemsize : descr.itemsize()).release().ptr();
+        m_ptr = descr.strip_padding(info.itemsize != 0 ? info.itemsize : descr.itemsize())
+                    .release()
+                    .ptr();
     }
 
     explicit dtype(const std::string &format) {
@@ -486,7 +488,7 @@ class dtype : public object {
     /// This is essentially the same as calling numpy.dtype(args) in Python.
     static dtype from_args(object args) {
         PyObject *ptr = nullptr;
-        if (!detail::npy_api::get().PyArray_DescrConverter_(args.ptr(), &ptr) || !ptr)
+        if ((detail::npy_api::get().PyArray_DescrConverter_(args.ptr(), &ptr) == 0) || !ptr)
             throw error_already_set();
         return reinterpret_steal<dtype>(ptr);
     }
@@ -542,7 +544,7 @@ class dtype : public object {
             auto name = spec[0].cast<pybind11::str>();
             auto format = spec[1].cast<tuple>()[0].cast<dtype>();
             auto offset = spec[1].cast<tuple>()[1].cast<pybind11::int_>();
-            if (!len(name) && format.kind() == 'V')
+            if ((len(name) == 0u) && format.kind() == 'V')
                 continue;
             field_descriptors.push_back({(PYBIND11_STR_TYPE) name, format.strip_padding(format.itemsize()), offset});
         }
@@ -872,11 +874,12 @@ template <typename T, int ExtraFlags = array::forcecast> class array_t : public
         : array(std::move(shape), std::move(strides), ptr, base) { }
 
     explicit array_t(ShapeContainer shape, const T *ptr = nullptr, handle base = handle())
-        : array_t(private_ctor{}, std::move(shape),
-                ExtraFlags & f_style
-                ? detail::f_strides(*shape, itemsize())
-                : detail::c_strides(*shape, itemsize()),
-                ptr, base) { }
+        : array_t(private_ctor{},
+                  std::move(shape),
+                  (ExtraFlags & f_style) != 0 ? detail::f_strides(*shape, itemsize())
+                                              : detail::c_strides(*shape, itemsize()),
+                  ptr,
+                  base) {}
 
     explicit array_t(ssize_t count, const T *ptr = nullptr, handle base = handle())
         : array({count}, {}, ptr, base) { }

include/pybind11/pybind11.h

@@ -318,7 +318,8 @@ class cpp_function : public function {
                 a.descr = guarded_strdup(repr(a.value).cast<std::string>().c_str());
         }
 
-        rec->is_constructor = !strcmp(rec->name, "__init__") || !strcmp(rec->name, "__setstate__");
+        rec->is_constructor
+            = (strcmp(rec->name, "__init__") == 0) || (strcmp(rec->name, "__setstate__") == 0);
 
 #if !defined(NDEBUG) && !defined(PYBIND11_DISABLE_NEW_STYLE_INIT_WARNING)
         if (rec->is_constructor && !rec->is_new_style_constructor) {
@@ -1131,7 +1132,8 @@ class generic_type : public object {
             pybind11_fail("generic_type: cannot initialize type \"" + std::string(rec.name) +
                           "\": an object with that name is already defined");
 
-        if (rec.module_local ? get_local_type_info(*rec.type) : get_global_type_info(*rec.type))
+        if ((rec.module_local ? get_local_type_info(*rec.type) : get_global_type_info(*rec.type))
+            != nullptr)
             pybind11_fail("generic_type: type \"" + std::string(rec.name) +
                           "\" is already registered!");
 
@@ -1209,8 +1211,9 @@ class generic_type : public object {
     void def_property_static_impl(const char *name,
                                   handle fget, handle fset,
                                   detail::function_record *rec_func) {
-        const auto is_static = rec_func && !(rec_func->is_method && rec_func->scope);
-        const auto has_doc = rec_func && rec_func->doc && pybind11::options::show_user_defined_docstrings();
+        const auto is_static = (rec_func != nullptr) && !(rec_func->is_method && rec_func->scope);
+        const auto has_doc = (rec_func != nullptr) && (rec_func->doc != nullptr)
+                             && pybind11::options::show_user_defined_docstrings();
         auto property = handle((PyObject *) (is_static ? get_internals().static_property_type
                                                        : &PyProperty_Type));
         attr(name) = property(fget.ptr() ? fget : none(),
@@ -2220,8 +2223,8 @@ inline function get_type_override(const void *this_ptr, const type_info *this_ty
        Unfortunately this doesn't work on PyPy. */
 #if !defined(PYPY_VERSION)
     PyFrameObject *frame = PyThreadState_Get()->frame;
-    if (frame && (std::string) str(frame->f_code->co_name) == name &&
-        frame->f_code->co_argcount > 0) {
+    if (frame != nullptr && (std::string) str(frame->f_code->co_name) == name
+        && frame->f_code->co_argcount > 0) {
         PyFrame_FastToLocals(frame);
         PyObject *self_caller = dict_getitem(
             frame->f_locals, PyTuple_GET_ITEM(frame->f_code->co_varnames, 0));

include/pybind11/pytypes.h

@@ -773,7 +773,11 @@ class dict_readonly {
     dict_readonly(handle obj, ssize_t pos) : obj(obj), pos(pos) { increment(); }
 
     reference dereference() const { return {key, value}; }
-    void increment() { if (!PyDict_Next(obj.ptr(), &pos, &key, &value)) { pos = -1; } }
+    void increment() {
+        if (PyDict_Next(obj.ptr(), &pos, &key, &value) == 0) {
+            pos = -1;
+        }
+    }
     bool equal(const dict_readonly &b) const { return pos == b.pos; }
 
 private:
@@ -1169,14 +1173,14 @@ class bool_ : public object {
     bool_() : object(Py_False, borrowed_t{}) { }
     // Allow implicit conversion from and to `bool`:
     bool_(bool value) : object(value ? Py_True : Py_False, borrowed_t{}) { }
-    operator bool() const { return m_ptr && PyLong_AsLong(m_ptr) != 0; }
+    operator bool() const { return (m_ptr != nullptr) && PyLong_AsLong(m_ptr) != 0; }
 
 private:
     /// Return the truth value of an object -- always returns a new reference
     static PyObject *raw_bool(PyObject *op) {
         const auto value = PyObject_IsTrue(op);
         if (value == -1) return nullptr;
-        return handle(value ? Py_True : Py_False).inc_ref().ptr();
+        return handle(value != 0 ? Py_True : Py_False).inc_ref().ptr();
     }
 };
 
@@ -1607,7 +1611,7 @@ inline memoryview memoryview::from_buffer(
     size_t ndim = shape->size();
     if (ndim != strides->size())
         pybind11_fail("memoryview: shape length doesn't match strides length");
-    ssize_t size = ndim ? 1 : 0;
+    ssize_t size = ndim != 0u ? 1 : 0;
     for (size_t i = 0; i < ndim; ++i)
         size *= (*shape)[i];
     Py_buffer view;

include/pybind11/stl/filesystem.h

@@ -67,15 +67,15 @@ template<typename T> struct path_caster {
         }
         PyObject* native = nullptr;
         if constexpr (std::is_same_v<typename T::value_type, char>) {
-            if (PyUnicode_FSConverter(buf, &native)) {
+            if (PyUnicode_FSConverter(buf, &native) != 0) {
                 if (auto c_str = PyBytes_AsString(native)) {
                     // AsString returns a pointer to the internal buffer, which
                     // must not be free'd.
                     value = c_str;
                 }
             }
         } else if constexpr (std::is_same_v<typename T::value_type, wchar_t>) {
-            if (PyUnicode_FSDecoder(buf, &native)) {
+            if (PyUnicode_FSDecoder(buf, &native) != 0) {
                 if (auto c_str = PyUnicode_AsWideCharString(native, nullptr)) {
                     // AsWideCharString returns a new string that must be free'd.
                     value = c_str;  // Copies the string.

tests/test_embed/test_interpreter.cpp

@@ -103,7 +103,7 @@ bool has_pybind11_internals_builtin() {
 
 bool has_pybind11_internals_static() {
     auto **&ipp = py::detail::get_internals_pp();
-    return ipp && *ipp;
+    return (ipp != nullptr) && (*ipp != nullptr);
 }
 
 TEST_CASE("Restart the interpreter") {

tests/test_methods_and_attributes.cpp

@@ -43,6 +43,7 @@ class ExampleMandA {
     void add6(int other) { value += other; }                        // passing by value
     void add7(int &other) { value += other; }                       // passing by reference
     void add8(const int &other) { value += other; }                 // passing by const reference
+    // NOLINTNEXTLINE(readability-non-const-parameter) Deliberately non-const for testing
     void add9(int *other) { value += *other; }                      // passing by pointer
     void add10(const int *other) { value += *other; }               // passing by const pointer
 

tests/test_numpy_dtypes.cpp

@@ -108,9 +108,11 @@ PYBIND11_PACKED(struct EnumStruct {
 
 std::ostream& operator<<(std::ostream& os, const StringStruct& v) {
     os << "a='";
-    for (size_t i = 0; i < 3 && v.a[i]; i++) os << v.a[i];
+    for (size_t i = 0; i < 3 && (v.a[i] != 0); i++)
+        os << v.a[i];
     os << "',b='";
-    for (size_t i = 0; i < 3 && v.b[i]; i++) os << v.b[i];
+    for (size_t i = 0; i < 3 && (v.b[i] != 0); i++)
+        os << v.b[i];
     return os << "'";
 }
 

tests/test_numpy_vectorize.cpp

@@ -59,7 +59,7 @@ TEST_SUBMODULE(numpy_vectorize, m) {
         .def(py::init<int>())
         .def_readwrite("value", &NonPODClass::value);
     m.def("vec_passthrough",
-          py::vectorize([](double *a,
+          py::vectorize([](const double *a,
                            double b,
                            // Changing this broke things
                            // NOLINTNEXTLINE(performance-unnecessary-value-param)

tests/test_smart_ptr.cpp

@@ -101,7 +101,7 @@ class MyObject3 : public std::enable_shared_from_this<MyObject3> {
 // test_unique_nodelete
 // Object with a private destructor
 class MyObject4;
-static std::unordered_set<MyObject4 *> myobject4_instances;
+std::unordered_set<MyObject4 *> myobject4_instances;
 class MyObject4 {
 public:
     MyObject4(int value) : value{value} {
@@ -127,7 +127,7 @@ class MyObject4 {
 // Object with std::unique_ptr<T, D> where D is not matching the base class
 // Object with a protected destructor
 class MyObject4a;
-static std::unordered_set<MyObject4a *> myobject4a_instances;
+std::unordered_set<MyObject4a *> myobject4a_instances;
 class MyObject4a {
 public:
     MyObject4a(int i) {

tests/test_stl.cpp

@@ -102,7 +102,7 @@ TEST_SUBMODULE(stl, m) {
     // test_set
     m.def("cast_set", []() { return std::set<std::string>{"key1", "key2"}; });
     m.def("load_set", [](const std::set<std::string> &set) {
-        return set.count("key1") && set.count("key2") && set.count("key3");
+        return (set.count("key1") != 0u) && (set.count("key2") != 0u) && (set.count("key3") != 0u);
     });
 
     // test_recursive_casting
@@ -196,9 +196,7 @@ TEST_SUBMODULE(stl, m) {
     m.def("double_or_zero", [](const opt_int& x) -> int {
         return x.value_or(0) * 2;
     });
-    m.def("half_or_none", [](int x) -> opt_int {
-        return x ? opt_int(x / 2) : opt_int();
-    });
+    m.def("half_or_none", [](int x) -> opt_int { return x != 0 ? opt_int(x / 2) : opt_int(); });
     m.def("test_nullopt", [](opt_int x) {
         return x.value_or(42);
     }, py::arg_v("x", std::nullopt, "None"));