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

Replace PROTECT with Shield<> #222

Merged
merged 2 commits into from
Apr 8, 2017
Merged

Replace PROTECT with Shield<> #222

merged 2 commits into from
Apr 8, 2017

Conversation

eddelbuettel
Copy link
Member

This addresses the concern by Tomas Kalibera (see here) and updates ChangeLog and NEWS.

@armstrtw
Copy link
Contributor

armstrtw commented Apr 7, 2017

I don't think we need this.

Do you think shield is better?

His check code doesn't really work for c++.

@eddelbuettel
Copy link
Member Author

Yes I do think Rcpp::Shield<> is better. It is also super-simple. No loss here.

But I agree that his check gets tripped up. He also sent me a false positive for the (old !!) RcppClassic where things happen correctly in the destructor but he didn't see that.

@eddelbuettel
Copy link
Member Author

The bigger story is of course that we could just as well

        ans = Rcpp::LogicalVector(n, NA_LOGICAL);

instead of

        ans = Rcpp::Shield<SEXP>(Rf_allocVector(LGLSXP,n));
        std::fill(LOGICAL(ans),LOGICAL(ans)+n,NA_LOGICAL);

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Apr 7, 2017

Ie

R> library(Rcpp)
R> cppFunction("LogicalVector makeOne(int n) { return LogicalVector(n, NA_LOGICAL); }")
R> makeOne(3)
[1] NA NA NA
R> class(makeOne(3))
[1] "logical"
R> 

@eddelbuettel
Copy link
Member Author

If you have a moment over the weekend I would appreciate it if you could glance at this.

@armstrtw
Copy link
Contributor

armstrtw commented Apr 8, 2017

Wil do. Just straight from work to dinner.

@armstrtw
Copy link
Contributor

armstrtw commented Apr 8, 2017

I'm looking at the source for shield.

It doesn't do out of order destruction. In this case, it doesn't really matter.

Why not use UNPROTECT_PTR ?

Anyway, his check is just bogus, so there's not really a need to make this change. but if you want to make the code more 'Rcppish' I'm ok with that.

@eddelbuettel eddelbuettel merged commit 0da689a into master Apr 8, 2017
@eddelbuettel eddelbuettel deleted the bugfix/rprotect branch April 8, 2017 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants