Page MenuHomePhabricator

[ldlb/SWIG] Refactor extensions to be non Python-specific.

Authored by JDevlieghere on Jan 7 2020, 5:14 PM.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 7 2020, 5:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 5:14 PM
JDevlieghere marked 2 inline comments as done.Jan 7 2020, 5:15 PM
JDevlieghere added inline comments.
4 ↗(On Diff #236722)

of course this should be factored out and reused across different SB classes.


This is unfortunate but not worse that what's already done for PythonDataObjects.h.

JDevlieghere marked an inline comment as done.Jan 7 2020, 10:04 PM
JDevlieghere added inline comments.
9 ↗(On Diff #236722)

I don't think this is the responsibility of the bindings, but for now I've kept it to make this a non-functional change.

Include ConstString.h once in extensions.swig.

labath added a comment.Jan 8 2020, 2:33 AM

I think this is a great direction to move in. If the extensions are written in C(++) then swig can automatically wrap everything we need.

I am wondering if we can avoid constifying each string we return this way though...

16–17 ↗(On Diff #236763)

Would it be possible to return std::string from here? I don't think this is really a public API (it's not accessible from C++, and swig should convert it into a native string before anything can get its hand on it), so I don't we need to be constrained by the SB API rules. And I believe swig already knows how to handle a std::string...

4 ↗(On Diff #236722)

Yes, but for now I'd put this code into SBTarget.i, since it's clearly SBTarget specific. If we have some generic extension code (produced by factoring this function for instance), then we can put it into some more generic place.

9 ↗(On Diff #236722)

FWIW, I don't find it particularly bad, especially when it gets factored into a function...

  • Use std::string
  • Move extension into the corresponding interface file
labath accepted this revision.Jan 8 2020, 11:36 AM




This revision is now accepted and ready to land.Jan 8 2020, 11:36 AM
This revision was automatically updated to reflect the committed changes.