Extend trampoline class pattern to support nesting #4547
Conversation
Chekov2k
force-pushed
the
multiple_inheritance_documentation
branch
5 times, most recently
from
b74490e
to
7953a56
Compare
Chekov2k
force-pushed
the
multiple_inheritance_documentation
branch
13 times, most recently
from
0c204ee
to
7909393
Compare
Chekov2k
force-pushed
the
multiple_inheritance_documentation
branch
from
7909393
to
cbb0590
Compare
| #define PYBIND11_OVERRIDE_TEMPLATE_NAME(base, ret_type, cname, name, fn, ...) \ | ||
| if (std::is_same<base, cname>::value) { \ | ||
| PYBIND11_OVERRIDE_PURE_NAME( \ | ||
| PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, fn, __VA_ARGS__); \ | ||
| } else { \ | ||
| PYBIND11_OVERRIDE_NAME( \ | ||
| PYBIND11_TYPE(ret_type), PYBIND11_TYPE(cname), name, fn, __VA_ARGS__); \ | ||
| } |
There was a problem hiding this comment.
This isn't a lot of meat, especially given the total size of this PR.
There is no significant complexity in this macro, and it's catering only to a niche case.
In contrast, the added documentation is overwhelming. If that's reflective of a typical use case, having to implement this helper macro is a relatively insignificant amount of extra code.
I don't see a lot of value in adding this pair of macros (this one and the one below).
There was a problem hiding this comment.
Thank you Ralf for having a look at my PR. I understand your sentiment and it is perfectly fine to not merge this. The only thing I would like to point out is that the first part of the PR is a pure documentation update. It shows a trampoline class pattern for combining two (or more) separate class inheritance
hierarchies avoiding code duplication (backed by some unit tests). Only the second part requires the new macros for reducing code duplication with pure virtual functions. Cheers Arnim
There was a problem hiding this comment.
I believe
- it's too much detail and too much code in that part of the documentation. (Especially because we're not set up to unit-test code in documentation.)
- that view is biased by: I wouldn't want to encourage creating that kind of complexity. I live more by "minimize use of multiple inheritance", it tends to get too confusing too quickly. (Which absolutely does not mean "never" btw.)
@henryiii do you think the code in the documentation under this PR could be a useful addition as an example somewhere?
There was a problem hiding this comment.
All valid points! Let me know what you decide and if things need moving around, stripping down, ...
Description
Extend documentation for multiple inheritance using trampoline classes. Adds a pattern to support nesting of two or more trampoline class inheritance hierarchies. Simple unit test showing the pattern has been added as well.
For some reason the MSVC linker needs a default implementation of the pure virtual base class methods in the unit tests. Should I add a pitfall section to the documentation? Something like:
Pitfall for MSVC: You might need to provide a hidden default implementation for the pure virtual base class methods due to missing symbols during linking, see https://devblogs.microsoft.com/oldnewthing/20131011-00/?p=2953Suggested changelog entry: