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

Inactive overload cache key #37

Conversation

BetsyMcPhail
Copy link

@BetsyMcPhail BetsyMcPhail commented Jan 15, 2020

Fixes issue reported in RobotLocomotion/drake#11424 using suggestion similar to RobotLocomotion/drake#11424 (comment).

Upstream bug report: pybind#1922

This change will replace #32.


This change is Reviewable

@BetsyMcPhail BetsyMcPhail force-pushed the inactive_overload_cache-key branch 2 times, most recently from 9fd21ae to 7204d70 Compare January 16, 2020 01:53
Copy link
Author

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):
+@EricCousineau-TRI for review


Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

:lgtm: with minor nits - thanks!

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Dismissed @BetsyMcPhail from a discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @BetsyMcPhail)

a discussion (no related file):
Testing locally, explicitly enabling the new test



include/pybind11/pybind11.h, line 2431 at r2 (raw file):

    /* N.B. This uses `__qualname__.name` instead of `name`
       to resolve pybind11#1922. */
    auto full_name = type.attr("__qualname__").cast<std::string>() + "." + name;

nit Can you describe this as std::string, since it's a pretty straightforward type?


include/pybind11/detail/internals.h, line 87 at r2 (raw file):

struct overload_hash {
    inline size_t operator()(const std::pair<const PyObject *, std::string>& v) const {
        size_t h1 = std::hash<const void *>()(v.first);

nit To minimize change, consider keeping the original semantics, size_t value = <first>; value ^= <second>; return value;?

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @BetsyMcPhail)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Testing locally, explicitly enabling the new test

Done. Test passed locally.


Instead of just the function name, use '__qualname__.name'.
@BetsyMcPhail BetsyMcPhail force-pushed the inactive_overload_cache-key branch from a95aa69 to db7b43f Compare January 17, 2020 21:58
Copy link
Author

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on @EricCousineau-TRI)


include/pybind11/pybind11.h, line 2431 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you describe this as std::string, since it's a pretty straightforward type?

Done


include/pybind11/detail/internals.h, line 87 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit To minimize change, consider keeping the original semantics, size_t value = <first>; value ^= <second>; return value;?

Done

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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