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

Use SWIG native support for boost::shared_ptr #142

Merged
merged 63 commits into from Mar 28, 2019

Conversation

@lballabio
Copy link
Owner

commented Oct 26, 2018

This PR is here for discussion rather than merging (and anyway, it's currently incomplete; I've done the conversion only for the Index hierarchy, and a lot more work is needed). (Update: Matthias Lungwitz did a part of that work.) (Update: the work is now complete.)

The background: in order to hide the use of shared_ptr from the users of the wrappers, we've been masking them in the SWIG interfaces (see https://www.implementingquantlib.com/2016/09/quantlib-and-swig.html for details).

Since a few years, however, this could be avoided by using the native support for shared_ptr provided by SWIG (http://swig.org/Doc3.0/Library.html#Library_std_shared_ptr).

The downside: we would have to drop Perl, since support for shared_ptr is not available for that language.

The upside: the SWIG interfaces would be greatly simplified. We would remove a number of dynamic_pointer_cast now required for getting to the correct pointer type, and we could declare methods of a derived class directly instead of %extending the masked pointer. See the changes in this PR for examples.

@mlungwitz

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Sounds good to me.

@pcaspers

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2018

The change would greatly simplify the SWIG interfaces for libraries interacting with QuantLib (like our QuantExt library) and would, therefore, be highly appreciated.

@mlungwitz

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2018

After playing around with the idea a little bit I have realized that we will run into issues with Swig-only classes such as PyObserver that break the original C++ inheritance chain. Do you already have some ideas in this direction, e.g. on how to adjust the instrument-based classes that are currently behind %ignore and in the swig C++ layer inherit from PyObserver?

@lballabio

This comment has been minimized.

Copy link
Owner Author

commented Nov 27, 2018

It should work out. We'll tell SWIG that those classes inherit from Observable, at which point it will allow them to be passed directly to observers for registration.

mlungwitz and others added some commits Nov 28, 2018

@lballabio

This comment has been minimized.

Copy link
Owner Author

commented Jan 3, 2019

Still to fix at this point:

  • some classes now use multiple inheritance, which is not supported by all languages: in those cases, we need to order the parent classes so that the one we can ignore comes second. Update: fixed by #149.
  • some inheritance relations are not mapped correctly (for instance, SWIG doesn't see that TypePayoff inherits from Payoff). Update: fixed by #147.
  • Some mappings to shared_ptr are not seen correctly, due to missing or circular %include in the sources (e.g., VanillaOption's constructor takes a shared_ptr<StrikedTypePayoff> before the latter is defined). Update: fixed by #149.
@mlungwitz

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Regarding multiple inheritance: Is the order of the parent classes obvious? I guess one has to judge which functionality provided by the parent classes is of higher relevance, right? Or do you plan to avoid multiple inheritance at all and use extends to provide the missing functionality via SWIG-code?

@lballabio

This comment has been minimized.

Copy link
Owner Author

commented Jan 17, 2019

Right, we'll have to decide from which class to inherit based on relevance. Methods from the second base class can be added back with %extend, but the inheritance relationship will be lost.

lballabio added some commits Mar 3, 2019

@lballabio lballabio referenced this pull request Mar 4, 2019

Merged

Enable Travis CI build. #153

lballabio added some commits Mar 5, 2019

Avoid marking base classes as abstract.
This would require declaring overridden methods in derived
classes in order to be able to instantiate them.

@lballabio lballabio merged commit de399ab into master Mar 28, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

lballabio added a commit that referenced this pull request Mar 28, 2019

@lballabio lballabio deleted the shared_ptr branch Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.