Skip to content

Feature/add more shields #1011

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 4, 2019
Merged

Feature/add more shields #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2ed04f
protect Rf_mkString inside Rf_lang2
eddelbuettel Nov 2, 2019
dd7b0d2
protect Rf_mkString inside Rf_lang2
eddelbuettel Nov 2, 2019
da4b705
protect Rf_mkString inside Rf_lang3 and Rf_lang4
eddelbuettel Nov 2, 2019
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
4 changes: 4 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

* DESCRIPTION (Version, Date): Roll minor version

* inst/include/Rcpp/Reference.h: Shield Rf_mkstring inside Rf_lang2
* inst/include/Rcpp/Environment.h: Idem (inside Rf_lang4)
* inst/include/Rcpp/proxy/FieldProxy.h: Idem (inside Rf_lang3 and 4)

2019-10-31 Romain Francois <romain@rstudio.com>

* inst/include/Rcpp/DataFrame.h: Protect temporaries from gc
Expand Down
9 changes: 4 additions & 5 deletions inst/include/Rcpp/Environment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
//
// Environment.h: Rcpp R/C++ interface class library -- access R environments
//
// Copyright (C) 2009 - 2013 Dirk Eddelbuettel and Romain Francois
// Copyright (C) 2014 Dirk Eddelbuettel, Romain Francois and Kevin Ushey
// Copyright (C) 2009 - 2013 Dirk Eddelbuettel and Romain Francois
// Copyright (C) 2014 - 2019 Dirk Eddelbuettel, Romain Francois and Kevin Ushey
//
// This file is part of Rcpp.
//
Expand Down Expand Up @@ -246,9 +246,8 @@ namespace Rcpp{
we have to go back to R to do this operation */
SEXP internalSym = Rf_install( ".Internal" );
SEXP removeSym = Rf_install( "remove" );
Shield<SEXP> call( Rf_lang2(internalSym,
Rf_lang4(removeSym, Rf_mkString(name.c_str()), Storage::get__(), Rf_ScalarLogical( FALSE ))
) );
Shield<SEXP> str(Rf_mkString(name.c_str()));
Shield<SEXP> call(Rf_lang2(internalSym, Rf_lang4(removeSym, str, Storage::get__(), Rf_ScalarLogical(FALSE))));
Copy link
Contributor

@kevinushey kevinushey Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It may also be worth pulling out Rf_lang4() from this call and protecting that as well, since IIUC that will be an unprotected R object being passed into Rf_lang2(), and so in theory could be cleaned up by the GC when the pairlist created in Rf_lang2() is allocated..

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I might be wrong. IIUC, each of the lang* helper functions protects the arguments passed in, before the final call is created:

https://github.com/wch/r-source/blob/eafae2b2ed39b3383943cd6c36f92d78f966811e/src/include/Rinlinedfuns.h#L629-L677

So this pattern should in fact be safe.

Rcpp_fast_eval( call, R_GlobalEnv ) ;
}
} else{
Expand Down
5 changes: 3 additions & 2 deletions inst/include/Rcpp/Reference.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// Reference.h: Rcpp R/C++ interface class library -- Reference class objects
//
// Copyright (C) 2010 - 2015 Dirk Eddelbuettel and Romain Francois
// Copyright (C) 2010 - 2019 Dirk Eddelbuettel and Romain Francois
//
// This file is part of Rcpp.
//
Expand Down Expand Up @@ -58,7 +58,8 @@ namespace Rcpp{
*/
Reference_Impl( const std::string& klass ) {
SEXP newSym = Rf_install("new");
Shield<SEXP> call( Rf_lang2( newSym, Rf_mkString( klass.c_str() ) ) );
Shield<SEXP> str(Rf_mkString(klass.c_str()));
Shield<SEXP> call(Rf_lang2(newSym, str));
Storage::set__( Rcpp_fast_eval( call , Rcpp::internal::get_Rcpp_namespace()) );
}

Expand Down
9 changes: 6 additions & 3 deletions inst/include/Rcpp/proxy/FieldProxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,14 @@ class FieldProxyPolicy {
const std::string& field_name ;

SEXP get() const {
Shield<SEXP> call( Rf_lang3( R_DollarSymbol, parent, Rf_mkString(field_name.c_str()) ) ) ;
Shield<SEXP> str(Rf_mkString(field_name.c_str()));
Shield<SEXP> call(Rf_lang3(R_DollarSymbol, parent, str));
return Rcpp_fast_eval( call, R_GlobalEnv ) ;
}
void set(SEXP x ) {
SEXP dollarGetsSym = Rf_install( "$<-");
Shield<SEXP> call( Rf_lang4( dollarGetsSym, parent, Rf_mkString(field_name.c_str()) , x ) ) ;
Shield<SEXP> str(Rf_mkString(field_name.c_str()));
Shield<SEXP> call(Rf_lang4(dollarGetsSym, parent, str, x));
parent.set__( Rcpp_fast_eval( call, R_GlobalEnv ) );
}
} ;
Expand All @@ -66,7 +68,8 @@ class FieldProxyPolicy {
const std::string& field_name ;

SEXP get() const {
Shield<SEXP> call( Rf_lang3( R_DollarSymbol, parent, Rf_mkString(field_name.c_str()) ) ) ;
Shield<SEXP> str(Rf_mkString(field_name.c_str()));
Shield<SEXP> call(Rf_lang3(R_DollarSymbol, parent, str));
return Rcpp_fast_eval( call, R_GlobalEnv ) ;
}
} ;
Expand Down