Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: clang-tidy upgrade (to version 18) #5272

Merged
merged 14 commits into from
Jul 29, 2024

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jul 29, 2024

Description

See individual commits for case-by-case background.

clang 18 testing is added to match clang-tidy 18.

clang 17 testing is added just to not leave a gap in clang versions.

Primary motivation for the upgrade at this time: Get access to misc-include-cleaner, which was added in clang version 17. (IWYU cleanup work will be done under a separate PR.)

Suggested changelog entry:

clang-tidy (in GitHub Actions) was updated from clang 15 to clang 18.

Ralf W. Grosse-Kunstleve added 14 commits July 28, 2024 19:10
…h-missing-default-case`

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu

tests/test_numpy_dtypes.cpp:212:5: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
… test_tagbased_polymorphic.cpp

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu

tests/test_tagbased_polymorphic.cpp:77:40: warning: The value '150' provided to the cast expression is not in the valid range of values for 'Kind' [clang-analyzer-optin.core.EnumCastOutOfRange]
This existing comment in pybind11/eigen/tensor.h

```
                // move, take_ownership don't make any sense for a ref/map:
```

is at odds with the `delete src;` three lines up.

In real-world client code `take_ownership` will not exist (unless the client code is untested and unused). I.e. the `delete` is essentially only useful to avoid leaks in the pybind11 unit tests.

While upgrading to clang-tidy 18, the warning below appeared. Apparently it is produced during LTO, and it appears difficult to suppress. Regardless, the best way to resolve this is to remove the `delete` and to simply make the test objects `static` in the unit test code.

________

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________

```
lto-wrapper: warning: using serial compilation of 3 LTRANS jobs
lto-wrapper: note: see the ‘-flto’ option documentation for more information
In function ‘cast_impl’,
    inlined from ‘cast’ at /mounted_pybind11/include/pybind11/eigen/tensor.h:414:25,
    inlined from ‘operator()’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:296:40,
    inlined from ‘_FUN’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:267:21:
/mounted_pybind11/include/pybind11/eigen/tensor.h:475:17: warning: ‘operator delete’ called on unallocated object ‘<anonymous>’ [-Wfree-nonheap-object]
  475 |                 delete src;
      |                 ^
/mounted_pybind11/include/pybind11/eigen/../pybind11.h: In function ‘_FUN’:
/mounted_pybind11/include/pybind11/eigen/../pybind11.h:297:75: note: declared here
  297 |                     std::move(args_converter).template call<Return, Guard>(cap->f),
      |                                                                           ^
```
…patible with the Catch2 `REQUIRE` macro (26 warnings like the one below).

________

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________

```
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:9: warning: chained comparison 'v0 <= v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
  127 |         REQUIRE(ret == 42);
      |         ^
/build/tests/catch/catch.hpp:17670:24: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:47: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:9: note: operand 'v0' is here
  127 |         REQUIRE(ret == 42);
      |         ^
/build/tests/catch/catch.hpp:17670:24: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:47: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                               ^~~~~~~~~~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:17: note: operand 'v1' is here
  127 |         REQUIRE(ret == 42);
      |                 ^
/build/tests/catch/catch.hpp:17670:90: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                                                                                          ^~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:70: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                                                      ^~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:24: note: operand 'v2' is here
  127 |         REQUIRE(ret == 42);
      |                        ^
/build/tests/catch/catch.hpp:17670:90: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                                                                                          ^~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:70: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                                                      ^~~~~~~~~~~
```
… warnings.

________
Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________
```
pybind11/detail/internals.h:556:53: warning: multilevel pointer conversion from 'internals **' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/detail/type_caster_base.h:431:20: warning: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/numpy.h:904:81: warning: multilevel pointer conversion from '_object *const *' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/numpy.h:1989:39: warning: multilevel pointer conversion from 'typename vectorize_arg<const double *>::type *' (aka 'const double **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/numpy.h:1989:39: warning: multilevel pointer conversion from 'typename vectorize_arg<const VectorizeTestClass *>::type *' (aka 'const VectorizeTestClass **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/stl/filesystem.h:75:44: warning: multilevel pointer conversion from 'PyObject **' (aka '_object **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/stl/filesystem.h:83:42: warning: multilevel pointer conversion from 'PyObject **' (aka '_object **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
```
… PyPy build errors:

```
In file included from /Users/runner/work/pybind11/pybind11/tests/test_stl.cpp:18:
/Users/runner/work/pybind11/pybind11/include/pybind11/stl/filesystem.h:75:17: error: no matching function for call to 'PyPyUnicode_FSConverter'
            if (PyUnicode_FSConverter(buf, reinterpret_cast<void *>(&native)) != 0) {
                ^~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:969:31: note: expanded from macro 'PyUnicode_FSConverter'
                              ^~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:970:17: note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'struct _object **' for 2nd argument
PyAPI_FUNC(int) PyUnicode_FSConverter(struct _object *arg0, struct _object **arg1);
                ^
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:969:31: note: expanded from macro 'PyUnicode_FSConverter'
                              ^
In file included from /Users/runner/work/pybind11/pybind11/tests/test_stl.cpp:18:
/Users/runner/work/pybind11/pybind11/include/pybind11/stl/filesystem.h:83:17: error: no matching function for call to 'PyPyUnicode_FSDecoder'
            if (PyUnicode_FSDecoder(buf, reinterpret_cast<void *>(&native)) != 0) {
                ^~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:971:29: note: expanded from macro 'PyUnicode_FSDecoder'
                            ^~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:972:17: note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'struct _object **' for 2nd argument
PyAPI_FUNC(int) PyUnicode_FSDecoder(struct _object *arg0, struct _object **arg1);
                ^
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:971:29: note: expanded from macro 'PyUnicode_FSDecoder'
                            ^
```
@rwgk rwgk marked this pull request as ready for review July 29, 2024 08:28
@rwgk rwgk requested a review from henryiii as a code owner July 29, 2024 08:28
@@ -469,9 +469,6 @@ struct type_caster<Eigen::TensorMap<Type, Options>,
parent_object = reinterpret_borrow<object>(parent);
break;

case return_value_policy::take_ownership:
delete src;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a change, wouldn't it have been causing a double delete somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a change, wouldn't it have been causing a double delete somewhere?

It's a change only superficially, but in reality it's more of a no-op:

Imagine someone writing new bindings and trying return_value_policy::take_ownership. The next thing they know, they get the exception from the // fallthrough below. The only thing that makes sense is to pick from the two accepted return value policies named in the exception.

Copy-pasting the commit 7bf9b82 message here for better visibility:

Fix inconsistent pybind11/eigen/tensor.h behavior:

This existing comment in pybind11/eigen/tensor.h

                // move, take_ownership don't make any sense for a ref/map:

is at odds with the delete src; three lines up.

In real-world client code take_ownership will not exist (unless the client code is untested and unused). I.e. the delete is essentially only useful to avoid leaks in the pybind11 unit tests.

While upgrading to clang-tidy 18, the warning below appeared. Apparently it is produced during LTO, and it appears difficult to suppress. Regardless, the best way to resolve this is to remove the delete and to simply make the test objects static in the unit test code.

lto-wrapper: warning: using serial compilation of 3 LTRANS jobs
lto-wrapper: note: see the ‘-flto’ option documentation for more information
In function ‘cast_impl’,
    inlined from ‘cast’ at /mounted_pybind11/include/pybind11/eigen/tensor.h:414:25,
    inlined from ‘operator()’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:296:40,
    inlined from ‘_FUN’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:267:21:
/mounted_pybind11/include/pybind11/eigen/tensor.h:475:17: warning: ‘operator delete’ called on unallocated object ‘<anonymous>’ [-Wfree-nonheap-object]
  475 |                 delete src;
      |                 ^
/mounted_pybind11/include/pybind11/eigen/../pybind11.h: In function ‘_FUN’:
/mounted_pybind11/include/pybind11/eigen/../pybind11.h:297:75: note: declared here
  297 |                     std::move(args_converter).template call<Return, Guard>(cap->f),
      |                                                                           ^

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay.

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 29, 2024

Thanks @henryiii and @Skylion007!

@rwgk rwgk merged commit 2e260b0 into pybind:master Jul 29, 2024
89 checks passed
@rwgk rwgk deleted the clang-tidy_upgrade_v18 branch July 29, 2024 18:10
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 29, 2024
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 30, 2024
…c_test.cpp

Follow-on to pybind#5272 — clang-tidy upgrade (to version 18)

Fixes this error:

```
/__w/pybind11/pybind11/tests/pure_cpp/smart_holder_poc_test.cpp:167:12: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [bugprone-unused-return-value,-warnings-as-errors]
  167 |     (void) new_owner.release(); // Manually verified: without this, clang++ -fsanitize=address
      |            ^~~~~~~~~~~~~~~~~~~
```

See also: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unused-return-value.html

Specifically there:

> std::unique_ptr::release(). Not using the return value can lead to resource leaks if the same pointer isn’t stored anywhere else. Often, ignoring the release() return value indicates that the programmer confused the function with reset().

I.e. the only way to tell clang-tidy that the smart_holder_poc_test.cpp is correct is to add the `NOLINT`.
rwgk pushed a commit to rwgk/pybind11clif that referenced this pull request Jul 30, 2024
henryiii pushed a commit that referenced this pull request Aug 12, 2024
* `container: silkeh/clang:18-bookworm` in .github/workflows/format.yml

* clang-tidy auto-fix (trivial, in test only)

* Disable `performance-enum-size` (noisy, low value)

* Temporarily turn off 3 diagnostics (to be tackled one-by-one).

* Add explicit `switch` `default` to resolve clang-tidy `bugprone-switch-missing-default-case`

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu

tests/test_numpy_dtypes.cpp:212:5: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]

* Add clang-17 and clang-18 testing.

* Add `NOLINTNEXTLINE(clang-analyzer-optin.core.EnumCastOutOfRange)` in test_tagbased_polymorphic.cpp

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu

tests/test_tagbased_polymorphic.cpp:77:40: warning: The value '150' provided to the cast expression is not in the valid range of values for 'Kind' [clang-analyzer-optin.core.EnumCastOutOfRange]

* Fix inconsistent pybind11/eigen/tensor.h behavior:

This existing comment in pybind11/eigen/tensor.h

```
                // move, take_ownership don't make any sense for a ref/map:
```

is at odds with the `delete src;` three lines up.

In real-world client code `take_ownership` will not exist (unless the client code is untested and unused). I.e. the `delete` is essentially only useful to avoid leaks in the pybind11 unit tests.

While upgrading to clang-tidy 18, the warning below appeared. Apparently it is produced during LTO, and it appears difficult to suppress. Regardless, the best way to resolve this is to remove the `delete` and to simply make the test objects `static` in the unit test code.

________

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________

```
lto-wrapper: warning: using serial compilation of 3 LTRANS jobs
lto-wrapper: note: see the ‘-flto’ option documentation for more information
In function ‘cast_impl’,
    inlined from ‘cast’ at /mounted_pybind11/include/pybind11/eigen/tensor.h:414:25,
    inlined from ‘operator()’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:296:40,
    inlined from ‘_FUN’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:267:21:
/mounted_pybind11/include/pybind11/eigen/tensor.h:475:17: warning: ‘operator delete’ called on unallocated object ‘<anonymous>’ [-Wfree-nonheap-object]
  475 |                 delete src;
      |                 ^
/mounted_pybind11/include/pybind11/eigen/../pybind11.h: In function ‘_FUN’:
/mounted_pybind11/include/pybind11/eigen/../pybind11.h:297:75: note: declared here
  297 |                     std::move(args_converter).template call<Return, Guard>(cap->f),
      |                                                                           ^
```

* Disable `bugprone-chained-comparison`: this clang-tidy check is incompatible with the Catch2 `REQUIRE` macro (26 warnings like the one below).
________

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________

```
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:9: warning: chained comparison 'v0 <= v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
  127 |         REQUIRE(ret == 42);
      |         ^
/build/tests/catch/catch.hpp:17670:24: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:47: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:9: note: operand 'v0' is here
  127 |         REQUIRE(ret == 42);
      |         ^
/build/tests/catch/catch.hpp:17670:24: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:47: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                               ^~~~~~~~~~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:17: note: operand 'v1' is here
  127 |         REQUIRE(ret == 42);
      |                 ^
/build/tests/catch/catch.hpp:17670:90: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                                                                                          ^~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:70: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                                                      ^~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:24: note: operand 'v2' is here
  127 |         REQUIRE(ret == 42);
      |                        ^
/build/tests/catch/catch.hpp:17670:90: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                                                                                          ^~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:70: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                                                      ^~~~~~~~~~~
```

* Add 8 `// NOLINT(bugprone-empty-catch)`

* Resolve clang-tidy `bugprone-multi-level-implicit-pointer-conversion` warnings.
________
Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________
```
pybind11/detail/internals.h:556:53: warning: multilevel pointer conversion from 'internals **' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/detail/type_caster_base.h:431:20: warning: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/numpy.h:904:81: warning: multilevel pointer conversion from '_object *const *' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/numpy.h:1989:39: warning: multilevel pointer conversion from 'typename vectorize_arg<const double *>::type *' (aka 'const double **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/numpy.h:1989:39: warning: multilevel pointer conversion from 'typename vectorize_arg<const VectorizeTestClass *>::type *' (aka 'const VectorizeTestClass **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/stl/filesystem.h:75:44: warning: multilevel pointer conversion from 'PyObject **' (aka '_object **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/stl/filesystem.h:83:42: warning: multilevel pointer conversion from 'PyObject **' (aka '_object **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
```

* Introduce `PYBIND11_REINTERPRET_CAST_VOID_PTR_IF_NOT_PYPY` to resolve PyPy build errors:

```
In file included from /Users/runner/work/pybind11/pybind11/tests/test_stl.cpp:18:
/Users/runner/work/pybind11/pybind11/include/pybind11/stl/filesystem.h:75:17: error: no matching function for call to 'PyPyUnicode_FSConverter'
            if (PyUnicode_FSConverter(buf, reinterpret_cast<void *>(&native)) != 0) {
                ^~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:969:31: note: expanded from macro 'PyUnicode_FSConverter'
                              ^~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:970:17: note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'struct _object **' for 2nd argument
PyAPI_FUNC(int) PyUnicode_FSConverter(struct _object *arg0, struct _object **arg1);
                ^
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:969:31: note: expanded from macro 'PyUnicode_FSConverter'
                              ^
In file included from /Users/runner/work/pybind11/pybind11/tests/test_stl.cpp:18:
/Users/runner/work/pybind11/pybind11/include/pybind11/stl/filesystem.h:83:17: error: no matching function for call to 'PyPyUnicode_FSDecoder'
            if (PyUnicode_FSDecoder(buf, reinterpret_cast<void *>(&native)) != 0) {
                ^~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:971:29: note: expanded from macro 'PyUnicode_FSDecoder'
                            ^~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:972:17: note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'struct _object **' for 2nd argument
PyAPI_FUNC(int) PyUnicode_FSDecoder(struct _object *arg0, struct _object **arg1);
                ^
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:971:29: note: expanded from macro 'PyUnicode_FSDecoder'
                            ^
```

* clang-tidy auto-fix

* Fix silly oversight.
@henryiii henryiii changed the title clang-tidy upgrade (to version 18) chore: clang-tidy upgrade (to version 18) Aug 13, 2024
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Aug 13, 2024
henryiii pushed a commit that referenced this pull request Aug 13, 2024
* `container: silkeh/clang:18-bookworm` in .github/workflows/format.yml

* clang-tidy auto-fix (trivial, in test only)

* Disable `performance-enum-size` (noisy, low value)

* Temporarily turn off 3 diagnostics (to be tackled one-by-one).

* Add explicit `switch` `default` to resolve clang-tidy `bugprone-switch-missing-default-case`

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu

tests/test_numpy_dtypes.cpp:212:5: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]

* Add clang-17 and clang-18 testing.

* Add `NOLINTNEXTLINE(clang-analyzer-optin.core.EnumCastOutOfRange)` in test_tagbased_polymorphic.cpp

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu

tests/test_tagbased_polymorphic.cpp:77:40: warning: The value '150' provided to the cast expression is not in the valid range of values for 'Kind' [clang-analyzer-optin.core.EnumCastOutOfRange]

* Fix inconsistent pybind11/eigen/tensor.h behavior:

This existing comment in pybind11/eigen/tensor.h

```
                // move, take_ownership don't make any sense for a ref/map:
```

is at odds with the `delete src;` three lines up.

In real-world client code `take_ownership` will not exist (unless the client code is untested and unused). I.e. the `delete` is essentially only useful to avoid leaks in the pybind11 unit tests.

While upgrading to clang-tidy 18, the warning below appeared. Apparently it is produced during LTO, and it appears difficult to suppress. Regardless, the best way to resolve this is to remove the `delete` and to simply make the test objects `static` in the unit test code.

________

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________

```
lto-wrapper: warning: using serial compilation of 3 LTRANS jobs
lto-wrapper: note: see the ‘-flto’ option documentation for more information
In function ‘cast_impl’,
    inlined from ‘cast’ at /mounted_pybind11/include/pybind11/eigen/tensor.h:414:25,
    inlined from ‘operator()’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:296:40,
    inlined from ‘_FUN’ at /mounted_pybind11/include/pybind11/eigen/../pybind11.h:267:21:
/mounted_pybind11/include/pybind11/eigen/tensor.h:475:17: warning: ‘operator delete’ called on unallocated object ‘<anonymous>’ [-Wfree-nonheap-object]
  475 |                 delete src;
      |                 ^
/mounted_pybind11/include/pybind11/eigen/../pybind11.h: In function ‘_FUN’:
/mounted_pybind11/include/pybind11/eigen/../pybind11.h:297:75: note: declared here
  297 |                     std::move(args_converter).template call<Return, Guard>(cap->f),
      |                                                                           ^
```

* Disable `bugprone-chained-comparison`: this clang-tidy check is incompatible with the Catch2 `REQUIRE` macro (26 warnings like the one below).
________

Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________

```
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:9: warning: chained comparison 'v0 <= v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
  127 |         REQUIRE(ret == 42);
      |         ^
/build/tests/catch/catch.hpp:17670:24: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:47: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:9: note: operand 'v0' is here
  127 |         REQUIRE(ret == 42);
      |         ^
/build/tests/catch/catch.hpp:17670:24: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:47: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                               ^~~~~~~~~~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:17: note: operand 'v1' is here
  127 |         REQUIRE(ret == 42);
      |                 ^
/build/tests/catch/catch.hpp:17670:90: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                                                                                          ^~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:70: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                                                      ^~~~~~~~~~~
/mounted_pybind11/tests/test_embed/test_interpreter.cpp:127:24: note: operand 'v2' is here
  127 |         REQUIRE(ret == 42);
      |                        ^
/build/tests/catch/catch.hpp:17670:90: note: expanded from macro 'REQUIRE'
 17670 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
       |                                                                                          ^~~~~~~~~~~
/build/tests/catch/catch.hpp:2710:70: note: expanded from macro 'INTERNAL_CATCH_TEST'
 2710 |             catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \
      |                                                                      ^~~~~~~~~~~
```

* Add 8 `// NOLINT(bugprone-empty-catch)`

* Resolve clang-tidy `bugprone-multi-level-implicit-pointer-conversion` warnings.
________
Debian clang version 18.1.8 (++20240718080534+3b5b5c1ec4a3-1~exp1~20240718200641.143)
Target: x86_64-pc-linux-gnu
________
```
pybind11/detail/internals.h:556:53: warning: multilevel pointer conversion from 'internals **' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/detail/type_caster_base.h:431:20: warning: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/numpy.h:904:81: warning: multilevel pointer conversion from '_object *const *' to 'const void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/numpy.h:1989:39: warning: multilevel pointer conversion from 'typename vectorize_arg<const double *>::type *' (aka 'const double **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/numpy.h:1989:39: warning: multilevel pointer conversion from 'typename vectorize_arg<const VectorizeTestClass *>::type *' (aka 'const VectorizeTestClass **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

pybind11/stl/filesystem.h:75:44: warning: multilevel pointer conversion from 'PyObject **' (aka '_object **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
pybind11/stl/filesystem.h:83:42: warning: multilevel pointer conversion from 'PyObject **' (aka '_object **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]
```

* Introduce `PYBIND11_REINTERPRET_CAST_VOID_PTR_IF_NOT_PYPY` to resolve PyPy build errors:

```
In file included from /Users/runner/work/pybind11/pybind11/tests/test_stl.cpp:18:
/Users/runner/work/pybind11/pybind11/include/pybind11/stl/filesystem.h:75:17: error: no matching function for call to 'PyPyUnicode_FSConverter'
            if (PyUnicode_FSConverter(buf, reinterpret_cast<void *>(&native)) != 0) {
                ^~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:969:31: note: expanded from macro 'PyUnicode_FSConverter'
                              ^~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:970:17: note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'struct _object **' for 2nd argument
PyAPI_FUNC(int) PyUnicode_FSConverter(struct _object *arg0, struct _object **arg1);
                ^
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:969:31: note: expanded from macro 'PyUnicode_FSConverter'
                              ^
In file included from /Users/runner/work/pybind11/pybind11/tests/test_stl.cpp:18:
/Users/runner/work/pybind11/pybind11/include/pybind11/stl/filesystem.h:83:17: error: no matching function for call to 'PyPyUnicode_FSDecoder'
            if (PyUnicode_FSDecoder(buf, reinterpret_cast<void *>(&native)) != 0) {
                ^~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:971:29: note: expanded from macro 'PyUnicode_FSDecoder'
                            ^~~~~~~~~~~~~~~~~~~~~
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:972:17: note: candidate function not viable: cannot convert argument of incomplete type 'void *' to 'struct _object **' for 2nd argument
PyAPI_FUNC(int) PyUnicode_FSDecoder(struct _object *arg0, struct _object **arg1);
                ^
/Users/runner/hostedtoolcache/PyPy/3.10.14/x64/include/pypy3.10/pypy_decl.h:971:29: note: expanded from macro 'PyUnicode_FSDecoder'
                            ^
```

* clang-tidy auto-fix

* Fix silly oversight.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants