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

git merge smart_holder #30139

Merged
merged 7 commits into from
Jul 30, 2024
Merged

git merge smart_holder #30139

merged 7 commits into from
Jul 30, 2024

Conversation

rwgk
Copy link
Contributor

@rwgk rwgk commented Jul 30, 2024

Description

Helper/scratch PR for testing.

Suggested changelog entry:

teo-tsirpanis and others added 7 commits July 23, 2024 22:32
* Fix typo.

* Remove outdated known limitation.

See google#5179.
* `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.
…c_test.cpp

Follow-on to pybind/pybind11#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 rwgk marked this pull request as ready for review July 30, 2024 05:03
@rwgk rwgk merged commit b5bbf2d into google:main Jul 30, 2024
155 checks passed
@rwgk rwgk deleted the pybind11k_merge_sh branch July 30, 2024 05:03
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.

2 participants