Closed rwgk closed 3 months ago
@henryiii It's past bedtime in my time zone. I'll look at the GHA results tomorrow.
Current thinking based on the hope that the GHA results look good:
With -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
(default is OFF
) we can be sure that we're not missing any specializations.
But full test coverage for all specializations is still missing. I believe it's at least a couple hours of leg work to achieve full coverage. Do we really need it? The specializations are pretty trivial: are code reviews sufficient?
@henryiii The PR description is complete now, with "Caveat" regarding unit test coverage.
I believe it's a minor caveat and we should include these bug fixes in the release as is. Or I could work on making the unit test coverage complete over the weekend, and we release next week.
Description
This PR adds a number of missing
handle_type_name
specializations that have accumulated over time unnoticed, until work under #4888 exposed the issue. This PR is based on #4888, but omits behavior changes other than the pure bug fixes.To prevent future accumulation of missing
handle_type_name
specializations, this PR adds the-DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION
cmake option, which isOFF
by default, but set toON
for some (but not all) ci.yml jobs. WhenON
, missinghandle_type_name
specializations trigger compilation errors.The
quote_cpp_type_name()
function introduced in #4888 is included here, but with the implementation changed to a no-op. This pin-points where C++ type names "slip through" (and could confuse stubgen), and enables easy future experimentation similar to #4888.Caveat: The test coverage for the
handle_type_name
specializations is incomplete: while the-DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION
option ensures that no specializations are missing, some of thename
s could be changed without triggering unit test failures.Suggested changelog entry: