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

fix #968 with a single argument XPtr constructor #1003

Merged
merged 2 commits into from Oct 20, 2019

Conversation

@stephematician
Copy link
Contributor

stephematician commented Oct 19, 2019

Cleaned up pull request 1002

as<> will no longer modify tags/protected data of an existing external pointer. Modifying the tags/protected data of an external pointer via the XPtr(SEXP,SEXP,SEXP) constructor is made more explicit by removing the two default arguments, and XPtr(SEXP) is implemented separately (as explicit) and does not modify the external pointer's tags/protected data.

Added a self-tag test which passed here. I was unable to test if downstream packages are affected by this change.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Oct 19, 2019

Somewhat semi-seriously, opening a new pull request is worse than cleaning the previous one (as it leads to issue inflation). All you needed was one git rm and add and commit and push. Too late now...

@stephematician

This comment has been minimized.

Copy link
Contributor Author

stephematician commented Oct 19, 2019

Somewhat, opening a new pull request is worse than cleaning the previous one (as it leads to issue inflation). All you needed was one git rm and add and commit and push. Too late now...

Roger that, apologies that you had to be the one to suffer my first PR in a long time.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Oct 19, 2019

No worries, after all it is you has to spend the rest of your life with the shame of a close Rcpp pull request ;-)

And thanks again for the fix. I am running reverse-depends on a seriously underpowered machine (but hey, free courtesy access on another continent so cannot really complain and am in fact rather grateful for having it) so it will be a few hours but zero issues through sixty packages so far.

@eddelbuettel eddelbuettel merged commit 1730b7f into RcppCore:master Oct 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Oct 20, 2019

@artemklevtsov Do you want to send me (or include here) a two-line diff?

@artemklevtsov

This comment has been minimized.

Copy link
Contributor

artemklevtsov commented Oct 20, 2019

# inst/unitTests/cpp/XPtr.cpp
+ return wrap(p) == (p).tag();
- return wrap(p) == R_ExternalPtrTag(p);

But I agree that R_ExternalPtrTag variant is more clear.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Oct 20, 2019

Sorry, when I say diff I mean something I can pass to patch and which fully and clearly specifies which file(s and line(s) are changed how. This only does the latter and leaves the other two unclear.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 6, 2019

@stephematician Slight trouble. On win-builder and with the older compiler I get

 D:/RCompile/CRANguest/R-devel/lib/Rcpp/include/Rcpp/XPtr.h: In constructor 'Rcpp::XPtr<T, StoragePolicy, Finalizer, finalizeOnExit>::XPtr(SEXP, SEXP, SEXP)':
  D:/RCompile/CRANguest/R-devel/lib/Rcpp/include/Rcpp/XPtr.h:81:56: warning: delegating constructors only available with -std=c++11 or -std=gnu++11
       explicit XPtr(SEXP x, SEXP tag, SEXP prot) : XPtr(x) {

and we have so far resisted the siren song of turning C++11 on in Rcpp itself.

Maybe we should condition part of this and/or keep the older version around?

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 6, 2019

Maybe we should condition part of this and/or keep the older version around?

Which is what I just did and will test. Your new change will be in effect if and only if we have C++11 or better (i.e. almost everywhere) and I stuck the previous ctor back in as a fallback for windoze and other old systems.

eddelbuettel added a commit that referenced this pull request Nov 7, 2019
eddelbuettel added a commit that referenced this pull request Nov 8, 2019
small updates incl hide away parts of #1003 if C++11 not available
@stephematician

This comment has been minimized.

Copy link
Contributor Author

stephematician commented Nov 13, 2019

Ah - right delegating constructors. Apologies for my slow response. We could move the work that the (delegating) constructor was doing into a private member, and call that in the body of both XPtr(SEXP) and XPtr(SEXP, SEXP, SEXP). That way there's no C++11-specific syntax, and we would gain consistency in XPtr across older and newer systems (although this seems like a real niche issue). I'll submit a PR along those lines, but otherwise I'm happy with the changes you've implemented.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 13, 2019

Oh, didn't think of that trick. Could do; or else we keep it simple and leave it as is ...

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.