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

use read-only variants of {STRING/VECTOR}_PTR #1317

Merged
merged 5 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
2024-07-07 Kevin Ushey <kevinushey@gmail.com>

* inst/include/Rcpp/internal/SEXP_Iterator.h: Avoid using VECTOR_PTR
* inst/include/Rcpp/vector/Subsetter.h: Avoid using STRING_PTR
* inst/include/RcppCommon.h: Include compatibility defines
* src/barrier.cpp: Avoid using {STRING/VECTOR}_PTR
* inst/include/Rcpp/r/compat.h: Include compatibility defines

2024-06-22 Dirk Eddelbuettel <edd@debian.org>

* DESCRIPTION (Version, Date): Roll micro version
Expand Down
2 changes: 1 addition & 1 deletion inst/include/Rcpp/internal/SEXP_Iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class SEXP_Iterator {

SEXP_Iterator( ): ptr(){} ;
SEXP_Iterator( const SEXP_Iterator& other) : ptr(other.ptr){} ;
SEXP_Iterator( const VECTOR& vec ) : ptr( get_vector_ptr(vec) ){} ;
SEXP_Iterator( const VECTOR& vec ) : ptr( RCPP_VECTOR_PTR(vec) ){} ;

SEXP_Iterator& operator=(const SEXP_Iterator& other){ ptr = other.ptr ; return *this ;}

Expand Down
37 changes: 37 additions & 0 deletions inst/include/Rcpp/r/compat.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@

//
// compat.h: Rcpp R/C++ interface class library -- compatibility defines
//
// Copyright (C) 2024 Dirk Eddelbuettel, Kevin Ushey
//
// This file is part of Rcpp.
//
// Rcpp is free software: you can redistribute it and/or modify it
// under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 2 of the License, or
// (at your option) any later version.
//
// Rcpp is distributed in the hope that it will be useful, but
// WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Rcpp. If not, see <http://www.gnu.org/licenses/>.

#ifndef RCPP_R_COMPAT_H
#define RCPP_R_COMPAT_H

#if defined(STRING_PTR_RO)
# define RCPP_STRING_PTR STRING_PTR_RO
#else
# define RCPP_STRING_PTR STRING_PTR
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. I wondered about the need to make this conditonal on existence of the new macro or min R version.


#if defined(VECTOR_PTR_RO)
# define RCPP_VECTOR_PTR VECTOR_PTR_RO
#else
# define RCPP_VECTOR_PTR VECTOR_PTR
#endif

#endif /* RCPP_R_COMPAT_H */
6 changes: 3 additions & 3 deletions inst/include/Rcpp/vector/Subsetter.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,10 @@ class SubsetProxy {
indices.reserve(rhs_n);
SEXP names = Rf_getAttrib(lhs, R_NamesSymbol);
if (Rf_isNull(names)) stop("names is null");
SEXP* namesPtr = STRING_PTR(names);
SEXP* rhsPtr = STRING_PTR(rhs);
const SEXP* namesPtr = RCPP_STRING_PTR(names);
const SEXP* rhsPtr = RCPP_STRING_PTR(rhs);
for (R_xlen_t i = 0; i < rhs_n; ++i) {
SEXP* match = std::find(namesPtr, namesPtr + lhs_n, *(rhsPtr + i));
const SEXP* match = std::find(namesPtr, namesPtr + lhs_n, *(rhsPtr + i));
if (match == namesPtr + lhs_n)
stop("not found");
indices.push_back(match - namesPtr);
Expand Down
1 change: 1 addition & 0 deletions inst/include/RcppCommon.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
// #define RCPP_DEBUG_MODULE_LEVEL 1

#include <Rcpp/r/headers.h>
#include <Rcpp/r/compat.h>

/**
* \brief Rcpp API
Expand Down
14 changes: 10 additions & 4 deletions src/barrier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@
#define COMPILING_RCPP

#define USE_RINTERNALS

#include <algorithm>
#include <Rinternals.h>

#include <Rcpp/barrier.h>
#include "internal.h"
#include <algorithm>
#include <Rcpp/protection/Shield.h>
#include <Rcpp/r/compat.h>

#include "internal.h"

// [[Rcpp::register]]
SEXP get_string_elt(SEXP x, R_xlen_t i) { // #nocov start
Expand All @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need a full rev.dep run to see if it is being called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we leave this code as-is for this PR, and then later test their removal in a separate PR, to see what breaks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan!

return const_cast<SEXP*>(RCPP_STRING_PTR(x));
}

// [[Rcpp::register]]
Expand All @@ -65,7 +70,8 @@ void set_vector_elt(SEXP x, R_xlen_t i, SEXP value) {

// [[Rcpp::register]]
SEXP* get_vector_ptr(SEXP x) {
return VECTOR_PTR(x); // #nocov end
// TODO: should we deprecate this?
return const_cast<SEXP*>(RCPP_VECTOR_PTR(x)); // #nocov end
}

// [[Rcpp::register]]
Expand Down
Loading