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.

Sign up for GitHub

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

[smart_holder] fix smart_holder regressions with multiple inheritance #3635

Merged
merged 1 commit into from
Jan 27, 2022
Merged

[smart_holder] fix smart_holder regressions with multiple inheritance #3635

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions include/pybind11/detail/smart_holder_type_casters.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ class modified_type_caster_generic_load_impl {
}
loaded_v_h = sub_caster.loaded_v_h;
loaded_v_h_cpptype = cast.first;
implicit_cast = cast.second;
// the sub_caster is being discarded, so steal its vector
implicit_casts = std::move(sub_caster.implicit_casts);
implicit_casts.emplace_back(cast.second);
return true;
}
}
Expand Down Expand Up @@ -149,7 +151,7 @@ class modified_type_caster_generic_load_impl {
}
loaded_v_h = foreign_loader->loaded_v_h;
loaded_v_h_cpptype = foreign_loader->loaded_v_h_cpptype;
implicit_cast = foreign_loader->implicit_cast;
implicit_casts = foreign_loader->implicit_casts; // SMART_HOLDER_WIP: should this be a copy or move?
return true;
}
return false;
Expand Down Expand Up @@ -251,7 +253,7 @@ class modified_type_caster_generic_load_impl {
const std::type_info *cpptype = nullptr;
void *unowned_void_ptr_from_direct_conversion = nullptr;
const std::type_info *loaded_v_h_cpptype = nullptr;
void *(*implicit_cast)(void *) = nullptr;
std::vector<void *(*)(void *)> implicit_casts;
value_and_holder loaded_v_h;
bool reinterpret_cast_deemed_ok = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the safety guard value change due to this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the safety guard value change due to this change?

I'm thinking not. The safety guard was meant to be a very targeted sanity check, not a version check.

I've ~neglected versioning of the smart_holder internals because Google-internally we're basically building everything from sources, for each test or executable independently & hermetically. But for more safety externally, It's probably a good idea to give smart_holder internal versioning a little more thought; in a separate PR.

I'll rerun the ubench timings asap. Not sure if I get a large-enough time window over the weekend, but Monday almost certainly.

// Magic number intentionally hard-coded, to guard against class_ holder mixups.
Expand Down Expand Up @@ -521,8 +523,10 @@ struct smart_holder_type_caster_load {

T *convert_type(void *void_ptr) const {
if (void_ptr != nullptr && load_impl.loaded_v_h_cpptype != nullptr
&& !load_impl.reinterpret_cast_deemed_ok && load_impl.implicit_cast != nullptr) {
void_ptr = load_impl.implicit_cast(void_ptr);
&& !load_impl.reinterpret_cast_deemed_ok && !load_impl.implicit_casts.empty()) {
for (auto implicit_cast: load_impl.implicit_casts) {
void_ptr = implicit_cast(void_ptr);
}
}
return static_cast<T *>(void_ptr);
}
Expand Down