-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Centralize readable function signatures to avoid duplication #5857
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
base: master
Are you sure you want to change the base?
Conversation
This seems to reduce size costs of adding enum_-specific implementations of dunder methods, but also should provide a nice to have size optimization for programs that use pybind11 in general.
Hmm, I could've sworn I saw Apple Clang 15 raise -Wdeprecated-redundant-constexpr-static-def, but here it is, claiming not to know about that warning: https://github.com/pybind/pybind11/actions/runs/18418181095/job/52486700109?pr=5857#step:11:123 Also, here is clang 16 calling it a plain old -Wdeprecated: https://github.com/pybind/pybind11/actions/runs/18418181095/job/52486699145?pr=5857#step:6:70 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. My suggestions are purely about naming and comments.
#define PYBIND11_READABLE_SIGNATURE_EXPR \ | ||
detail::const_name("(") + cast_in::arg_names + detail::const_name(") -> ") + cast_out::name | ||
|
||
// We centralize readable function signatures in a specific template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "centralize" here was throwing me off when I first looked at this PR. It led me to think you're making the source code more DRY, but apparently that's not what this PR does?
How about writing
// We factor out readable function signatures ...
?
// so that we don't duplicate them across different ...
Is it really "we"? It'd be the compiler? But a smart-enough compiler could do that regardless, maybe that's why some .so
s are still the same size?
How about
// so that we can be sure compilers don't duplicate them across different ...
}; | ||
#undef PYBIND11_READABLE_SIGNATURE_EXPR | ||
|
||
// Prior to C++17, we don't have inline variables, so we have to provide an out-of-line definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's a pretty big block of pure ugliness, even by the usual C++ standards ...
Could you please add a comment to prominently suggest that this block is removed when C++11 and C++ 14 support is dropped?
# define PYBIND11_COMPAT_STRDUP strdup | ||
#endif | ||
|
||
#define PYBIND11_READABLE_SIGNATURE_EXPR \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PYBIND11_READABLE_FUNCTION_SIGNATURE_EXPR
for consistency. — Since this is used only in a couple places, I think consistency is more valuable than trimming the length of the name.
template <typename cast_in, typename cast_out> | ||
class ReadableFunctionSignature { | ||
public: | ||
using readable_signature_type = decltype(PYBIND11_READABLE_SIGNATURE_EXPR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within this scope shorter names would make this code more readable, e.g. simply sig_type
here, and static constexpr sig_type sig()
, kSig
or similar below.
|
||
public: | ||
static constexpr readable_signature_type kReadableSignature = readable_signature(); | ||
#if !defined(_MSC_VER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment to explain (or at least hint) why this works only with compilers other than MSVC?
Description
This should provide a nice to have size optimization for programs that use pybind11 in general. I wrote it because it seems to reduce size costs of adding enum_-specific implementations of dunder methods, but since it seems generally useful, I thought it might be a good idea to send as a separate PR.
Concretely, for the pybind11_tests.cpython-313-darwin.so file built as part of pybind11's tests, I see the following size delta (output is
sdiff
ofsize -m
):Full disclosure: Not all of the other .sos improved (though none regressed). For this to show a benefit, users' pybind11 extensions would have to define multiple pybind11 functions or methods with the same signature.
Suggested changelog entry: