diff --git a/ChangeLog b/ChangeLog index e46bdd3c9..321ec0aa9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2024-06-11 Kevin Ushey + + * inst/include/Rcpp/internal/r_vector.h: Use template specializations to + avoid DATAPTR usage + * inst/include/Rcpp/vector/traits.h: Implement bounds checks in + r_vector_cache access + * inst/tinytest/cpp/Vector.cpp: Add unit tests + * inst/tinytest/test_vector.R: Add unit tests + * tests/tinytest.R: Test in serial by default + 2024-06-02 Dirk Eddelbuettel * inst/include/Rcpp/internal/export.h: More R_xlen_t switching diff --git a/inst/NEWS.Rd b/inst/NEWS.Rd index 88e36c3c7..1fbef846d 100644 --- a/inst/NEWS.Rd +++ b/inst/NEWS.Rd @@ -9,9 +9,14 @@ \itemize{ \item Set R_NO_REMAP if not already defined (Dirk in \ghpr{1296}) \item Add variadic templates to be used instead of generated code - (Andrew Johnson in \ghpr{1303} + (Andrew Johnson in \ghpr{1303}) \item Count variables were switches to \code{size_t} to avoid warnings about conversion-narrowing (Dirk in \ghpr{1307}) + \item Rcpp now avoids the usage of the (non-API) DATAPTR function when + accessing the contents of Rcpp Vector objects where possible. (Kevin in + \ghpr{1310}) + \item Rcpp now emits an R warning on out-of-bounds Vector accesses. This + may become an error in a future Rcpp release. (Kevin in \ghpr{1310}) } \item Changes in Rcpp Deployment: \itemize{ diff --git a/inst/include/Rcpp/internal/r_vector.h b/inst/include/Rcpp/internal/r_vector.h index efdf3c8e3..589c75fed 100644 --- a/inst/include/Rcpp/internal/r_vector.h +++ b/inst/include/Rcpp/internal/r_vector.h @@ -32,6 +32,21 @@ typename Rcpp::traits::storage_type::type* r_vector_start(SEXP x) { return reinterpret_cast(dataptr(x)); } +// add specializations to avoid use of dataptr +#define RCPP_VECTOR_START_IMPL(__RTYPE__, __ACCESSOR__) \ + template <> \ + inline typename Rcpp::traits::storage_type<__RTYPE__>::type* r_vector_start<__RTYPE__>(SEXP x) { \ + return __ACCESSOR__(x); \ + } + +RCPP_VECTOR_START_IMPL(LGLSXP, LOGICAL); +RCPP_VECTOR_START_IMPL(INTSXP, INTEGER); +RCPP_VECTOR_START_IMPL(RAWSXP, RAW); +RCPP_VECTOR_START_IMPL(CPLXSXP, COMPLEX); +RCPP_VECTOR_START_IMPL(REALSXP, REAL); + +#undef RCPP_VECTOR_START_IMPL + /** * The value 0 statically casted to the appropriate type for * the given SEXP type diff --git a/inst/include/Rcpp/vector/Subsetter.h b/inst/include/Rcpp/vector/Subsetter.h index 0d8b32a6a..339d2000d 100644 --- a/inst/include/Rcpp/vector/Subsetter.h +++ b/inst/include/Rcpp/vector/Subsetter.h @@ -133,9 +133,9 @@ class SubsetProxy { private: - #ifndef RCPP_NO_BOUNDS_CHECK template void check_indices(IDX* x, R_xlen_t n, R_xlen_t size) { +#ifndef RCPP_NO_BOUNDS_CHECK for (IDX i=0; i < n; ++i) { if (x[i] < 0 or x[i] >= size) { if(std::numeric_limits::is_integer && size > std::numeric_limits::max()) { @@ -144,11 +144,8 @@ class SubsetProxy { stop("index error"); } } +#endif } - #else - template - void check_indices(IDX* x, IDX n, IDX size) {} - #endif void get_indices( traits::identity< traits::int2type > t ) { indices.reserve(rhs_n); diff --git a/inst/include/Rcpp/vector/traits.h b/inst/include/Rcpp/vector/traits.h index 1e01d93dc..c6d76d694 100644 --- a/inst/include/Rcpp/vector/traits.h +++ b/inst/include/Rcpp/vector/traits.h @@ -35,22 +35,36 @@ namespace traits{ typedef typename r_vector_const_proxy::type const_proxy ; typedef typename storage_type::type storage_type ; - r_vector_cache() : start(0){} ; + r_vector_cache() : start(0), size(0) {} ; + inline void update( const VECTOR& v ) { - start = ::Rcpp::internal::r_vector_start(v) ; + start = ::Rcpp::internal::r_vector_start(v) ; + size = v.size(); } + inline iterator get() const { return start; } inline const_iterator get_const() const { return start; } - inline proxy ref() { return *start ;} - inline proxy ref(R_xlen_t i) { return start[i] ; } + inline proxy ref() { check_index(0); return start[0] ;} + inline proxy ref(R_xlen_t i) { check_index(i); return start[i] ; } + + inline proxy ref() const { check_index(0); return start[0] ;} + inline proxy ref(R_xlen_t i) const { check_index(i); return start[i] ; } - inline proxy ref() const { return *start ;} - inline proxy ref(R_xlen_t i) const { return start[i] ; } + private: - private: - iterator start ; + void check_index(R_xlen_t i) const { +#ifndef RCPP_NO_BOUNDS_CHECK + if (i >= size) { + warning("subscript out of bounds (index %s >= vector size %s)", i, size); + } +#endif + } + + iterator start ; + R_xlen_t size ; } ; + template class StoragePolicy = PreserveStorage> class proxy_cache{ public: @@ -66,17 +80,24 @@ namespace traits{ p = const_cast(&v) ; } inline iterator get() const { return iterator( proxy(*p, 0 ) ) ;} - // inline const_iterator get_const() const { return const_iterator( *p ) ;} inline const_iterator get_const() const { return const_iterator( const_proxy(*p, 0) ) ; } - inline proxy ref() { return proxy(*p,0) ; } - inline proxy ref(R_xlen_t i) { return proxy(*p,i);} + inline proxy ref() { check_index(0); return proxy(*p,0) ; } + inline proxy ref(R_xlen_t i) { check_index(i); return proxy(*p,i);} - inline const_proxy ref() const { return const_proxy(*p,0) ; } - inline const_proxy ref(R_xlen_t i) const { return const_proxy(*p,i);} + inline const_proxy ref() const { check_index(0); return const_proxy(*p,0) ; } + inline const_proxy ref(R_xlen_t i) const { check_index(i); return const_proxy(*p,i);} private: VECTOR* p ; + + void check_index(R_xlen_t i) const { +#ifndef RCPP_NO_BOUNDS_CHECK + if (i >= p->size()) { + warning("subscript out of bounds (index %s >= vector size %s)", i, p->size()); + } +#endif + } } ; // regular types for INTSXP, REALSXP, ... diff --git a/inst/tinytest/cpp/Vector.cpp b/inst/tinytest/cpp/Vector.cpp index 495796958..4fc26b7e8 100644 --- a/inst/tinytest/cpp/Vector.cpp +++ b/inst/tinytest/cpp/Vector.cpp @@ -886,3 +886,13 @@ bool CharacterVector_test_equality_crosspolicy(CharacterVector x, Vector