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

execute embedded R script in specified env #852

Merged
merged 3 commits into from May 26, 2018

Conversation

Projects
None yet
2 participants
@filipsch
Copy link
Contributor

filipsch commented May 25, 2018

Fixes #851

As mentioned in the issue, I have no idea about how to include a unit test to verify the new behavior; I am open to suggestions or somebody else adding it. Thanks!

I checked locally and it works:

test.rcpp

#include <Rcpp.h>
using namespace Rcpp ;

// [[Rcpp::export]]
int answer(){
    return 42 ;
}

/*** R
    # call answer and check you get the right result
    x <- answer()
    x
*/

in R:

> library(Rcpp)
> testEnv <- new.env()
> sourceCpp("test.cpp", env = testEnv)

>     # call answer and check you get the right result
>     x <- answer()

>     x
[1] 42
> ls(testEnv)
[1] "answer" "x"  
@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented May 25, 2018

For unit tests, please have a look at inst/unitTests/runit.*.R and inst/unitTests/cpp/*. Not all that complicated---R functions calling C++ code built via Rcpp Attributes. Just like you did in your reprex. Or am I missing something here while I barely started the first coffee cup of the day?

@filipsch

This comment has been minimized.

Copy link
Contributor Author

filipsch commented May 25, 2018

@eddelbuettel Thanks for the pointers, I'll see how far I get with those and will report here!

Filip Schouwenaars
@filipsch

This comment has been minimized.

Copy link
Contributor Author

filipsch commented May 25, 2018

@eddelbuettel Added a test, that fails if the local = env portion is not there.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented May 25, 2018

In that case could you actually have it fail and catch the failure?

We have plenty of other tests catching exceptions and errors; RUnit supports that.

@filipsch

This comment has been minimized.

Copy link
Contributor Author

filipsch commented May 25, 2018

@eddelbuettel in this particular case, I think it is hard to do: It would involve me overriding the source() function to ignore the local argument, not taking in the env parameter as such. Feels like a lot of trickery.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented May 25, 2018

Maybe. Methinks you could mod your example from #851 to provide the "wrong" answer() function and check for the wrong result. Or maybe not. Have not thought to hard about this.

Took a number of years for someone to come along and declare this a problem :)

@filipsch

This comment has been minimized.

Copy link
Contributor Author

filipsch commented May 25, 2018

I'm not sure if I understand. This fix makes sure that embedded R code is executed in the R environment in which the RCpp-generated functions are made available, instead of the default globalenv().
To check that this errs, I would have to programmatically update the R source of sourceCpp() and not take in the env argument. I don't see another way. Doing this feels very contrived.
Not sure if there's anymore I can contribute :)

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented May 25, 2018

My thought was that you don't have to have the R code of your C++ snippet executed via the sourceCpp() functionality you so like (and which is great!). Use sourceCpp() to compile a new function in your new environment -- the whole point of #851 as I understand it -- and then in the normal RUnit R code execution look for it. It should fail as it should be tucked away in the new environment. No?

Filip Schouwenaars
@filipsch

This comment has been minimized.

Copy link
Contributor Author

filipsch commented May 25, 2018

It's actually the other way around. The point of #851 is to source the R code in my new environment. Making available the Rcpp functions works fine already.
I've added a spec accordingly that fails as expected.
Thanks for your pointers!

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented May 26, 2018

I went all-in and did a reverse depends check, with was (as expected) without issues, so folding this in.

@eddelbuettel eddelbuettel merged commit 8c360a8 into RcppCore:master May 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.