use read-only variants of {STRING/VECTOR}_PTR #1317
Conversation
| # define RCPP_STRING_PTR STRING_PTR_RO | ||
| #else | ||
| # define RCPP_STRING_PTR STRING_PTR | ||
| #endif |
There was a problem hiding this comment.
Very nice. I wondered about the need to make this conditonal on existence of the new macro or min R version.
| @@ -50,7 +54,8 @@ void char_set_string_elt(SEXP x, R_xlen_t i, const char* value) { | |||
|
|
|||
| // [[Rcpp::register]] | |||
| SEXP* get_string_ptr(SEXP x) { | |||
| return STRING_PTR(x); | |||
| // TODO: should we deprecate this? | |||
There was a problem hiding this comment.
May need a full rev.dep run to see if it is being called?
There was a problem hiding this comment.
Maybe we leave this code as-is for this PR, and then later test their removal in a separate PR, to see what breaks?
|
Reverse-depends check running. Looks good so far but many more packages to go ... Update 1 @ 3.6h in: 177 pass, 0 fail Update 2 @ 13.6h in: 781 pass, 1 known-not-new fail Update 3 @ 24h in: 1410 pass, no new regression (3 fail for 'other' reasons, 14 skip) Update 4 on 07/10: This finished without new issue but we realized we need a second run one which is now running. Update 5 @ 15.6h: 1540 successes, one known fail and three fail for lack of depends, 14 skipped Update 6 @ 31.1h (thanks, So will merge and proceed with release. |
|
@kevinushey I just ran an edd@rob:~/git/rcpp(bugfix/vector-string-ptr-read-only)$ git ls|head -4
* edcf98f8 - (HEAD -> bugfix/vector-string-ptr-read-only, origin/bugfix/vector-string-ptr-read-only) one more const (30 hours ago) <Kevin Ushey>
* 1cb485b5 - another const_cast (31 hours ago) <Kevin Ushey>
* cd0f41ac - use read-only variants of {STRING/VECTOR}_PTR (31 hours ago) <Kevin Ushey>
* 51896112 - (origin/master, origin/HEAD, master) Release 1.0.12.4 (2 weeks ago) <Dirk Eddelbuettel>
edd@rob:~/git/rcpp(bugfix/vector-string-ptr-read-only)$ I named that 1.0.12.4.1 and it is the one I am testing now but: edd@rob:~/git/rcpp(bugfix/vector-string-ptr-read-only)$ rdcc.sh Rcpp_1.0.12.4.1.tar.gz
* using log directory ‘/tmp/r/Rcpp.Rcheck’
* using R Under development (unstable) (2024-07-07 r86877)
* using platform: x86_64-pc-linux-gnu
* R was compiled by
gcc (Ubuntu 13.2.0-4ubuntu3) 13.2.0
GNU Fortran (Ubuntu 13.2.0-4ubuntu3) 13.2.0
* running under: Ubuntu 23.10
* using session charset: UTF-8
* using option ‘--as-cran’
* checking for file ‘Rcpp/DESCRIPTION’ ... OK
* this is package ‘Rcpp’ version ‘1.0.12.4.1’
[....]
* checking compiled code ... NOTE
File ‘Rcpp/libs/Rcpp.so’:
Found non-API calls to R: ‘STRING_PTR’, ‘VECTOR_PTR’
Compiled code should not call non-API entry points in R.
See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual,
and section ‘Moving into C API compliance’ for issues with the use of non-API entry points.
* checking sizes of PDF files under ‘inst/doc’ ... OKAny idea why? Is that an error in the string matching / grepping? Should we not call them RCPP_{STRING,VECTOR}_PTR ? PS: Hm. Running |
| @@ -112,7 +112,7 @@ namespace Rcpp { | |||
| case BCODESXP: return "BCODESXP"; | |||
| case EXTPTRSXP: return "EXTPTRSXP"; | |||
| case WEAKREFSXP: return "WEAKREFSXP"; | |||
| #if R_Version >= R_Version(4,4,0) // replaces S4SXP in R 4.4.0 | |||
| #if R_VERSION >= R_Version(4,4,0) // replaces S4SXP in R 4.4.0 | |||
There was a problem hiding this comment.
This one seemed a bit strange; I'm not sure if this really worked before (or, if it did, why it did)?
There was a problem hiding this comment.
Yeah that looks like you spotted an 'ooops'. Thank you!
And thanks for the update. It look good. Will have to run another rev.dep run but we should be good by end of week. Fingers crossed!
Pull Request Template for Rcpp
Closes #1316. I tried to do this in a way that lets us use the read-only variants with newer versions of R, and fall back to the non-API variants for older versions of R.
Checklist
R CMD checkstill passes all tests