Skip to content

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Feb 20, 2015

Before we needed to have

    Environment ns = Environment::namespace_env("Rcpp") ;
    Function fun = ns["sourceRcpp"];

With this we can just do

    Function fun("sourceRcpp", "Rcpp");

@jjallaire
Copy link
Member

I believe that this change will break the ABI (because it adds overloads to the constructor). We can be 100% sure by running something like the ABI compliance checker (https://github.com/lvc/abi-compliance-checker/), which we should probably add to our test suite.

In any case, we don't have a policy that says we will never break the ABI, but we would like to do it all at once (collecting many ABI breaking changes) so as to minimize/contain the disruption this causes (a lot of mysterious crashing when users update Rcpp but not the packages that depend on it).

If this does in fact break the ABI we should do one of two things:

  1. Submit another change that doesn't break it (i.e. add global functions which construct function objects in the manner you are hoping for); or

  2. Give the PR an "ABI" tag and wait to merge it until we have other worthy ABI breaking changes.

@lionel-
Copy link
Contributor Author

lionel- commented Feb 20, 2015

oh ok. I'm new to compiled languages so I'm not familiar with this ABI stuff.

I'm ok with waiting for another ABI breaking changes.

@eddelbuettel
Copy link
Member

Last time we accidentally broke the ABI I tried to add a test with a prebuilt package. That was a good idea "in theory" but I (still) don;t know if it catches enough stuff "in practice".

@jjallaire jjallaire added the abi label Feb 20, 2015
@jjallaire
Copy link
Member

I think that approach will only catch ABI breakages affecting the code in the prebuilt package. So it would have only catched this one if the code in the prebuilt package calls the Function constructor.

@eddelbuettel
Copy link
Member

Yes, that makes a lot of sense. Good old (lack of) coverage problem.

@jjallaire
Copy link
Member

It's possible that there is no ABI breakage after-all since these changes are all within header files. Once we get a bit more tooling in place for this we can revisit the PR and take it if there is no ABI change.

jjallaire added a commit that referenced this pull request Feb 21, 2015
Function constructors that look up functions in an environment or in a namespace
@jjallaire jjallaire merged commit db6dae8 into RcppCore:master Feb 21, 2015
@jjallaire
Copy link
Member

The fact that these changes are header-only do indeed make them ABI safe.

@lionel-
Copy link
Contributor Author

lionel- commented Feb 21, 2015

great :-)

@lionel- lionel- deleted the fun_cons branch February 21, 2015 22:19
jmp75 pushed a commit to jmp75/Rcpp that referenced this pull request Mar 16, 2015
Function constructors that look up functions in an environment or in a namespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants