Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-9018

Investigate downcasting of Table and Record types in pybind11

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      2
    • Sprint:
      DRP S17-4
    • Team:
      Data Release Production

      Description

      pybind11 usually casts base-class references or pointers to the most-derived wrapped type, using RTTI and dynamic casts. This does not seem to work for Table and Record objects, where we instead have to wrap derived-class accessors that return the appropriate type.

      I suspect this is because the true types are defined in anonymous namespaces in C++ files, so the most-derived types are not actually accessible to pybind11, and it's doing some sort of dict lookup on std::type_info rather than a tree of dynamic_casts (what Boost.Python did). If that's the case, a invisible change to how these classes are implemented could clean up our pybind11 wrappers. I imagine we wouldn't want to try to fix this upstream, as pybind11's approach is almost certainly faster than Boost.Python's and works as well in the vast majority of cases.

        Attachments

          Issue Links

            Activity

            Hide
            pschella Pim Schellart [X] (Inactive) added a comment -

            Can we move this to cleanup epic?

            Show
            pschella Pim Schellart [X] (Inactive) added a comment - Can we move this to cleanup epic?
            Hide
            jbosch Jim Bosch added a comment -

            Yes, that's probably appropriate.

            Show
            jbosch Jim Bosch added a comment - Yes, that's probably appropriate.
            Hide
            jbosch Jim Bosch added a comment -

            This was getting in the way of some other cleanups on DM-8716, so I tested my proposed fix on that branch and it worked. I'm going to add this to the current sprint, put it back in the main pybind11 epic, and close it now even though the changes won't land until DM-8716 is in (which should be tomorrow barring some major disaster).

            Show
            jbosch Jim Bosch added a comment - This was getting in the way of some other cleanups on DM-8716 , so I tested my proposed fix on that branch and it worked. I'm going to add this to the current sprint, put it back in the main pybind11 epic, and close it now even though the changes won't land until DM-8716 is in (which should be tomorrow barring some major disaster).

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Watchers:
              Jim Bosch, Krzysztof Findeisen, Pim Schellart [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.