Skip to content

Conversation

@eddelbuettel
Copy link
Member

This PR updates #nocov tags for a number of R and C++ source files. Apart from one remaining 'function factory' function that confuses the coverage counting, this pushes the needle towards full coverage.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

Copy link
Member

@Enchufa2 Enchufa2 left a comment

Choose a reason for hiding this comment

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

Some comments below.

// This is used internally instead of Rf_eval() to make evaluation safer
inline SEXP Rcpp_eval_impl(SEXP expr, SEXP env) {
return Rcpp_fast_eval(expr, env);
inline SEXP Rcpp_eval_impl(SEXP expr, SEXP env) { // #nocov
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we drop this and use Rcpp_fast_eval directly in the remaining spots?


/* check if we can use a fast version */
if( TYPEOF(x) == STRSXP && parent.size() == Rf_length(x) ){
if( TYPEOF(x) == STRSXP && parent.size() == Rf_length(x) ){ // #nocov start
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should design a test to check the other side of the branch instead?

try{
index = parent.offset(name) ;
parent[ index ] = rhs ;
parent[ index ] = rhs ; // #nocov
Copy link
Member

Choose a reason for hiding this comment

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

Weird. Does this mean that we don't have any test that doesn't throw here?

/* We need to evaluate if it is a promise */
if( TYPEOF(res) == PROMSXP){
res = internal::Rcpp_eval_impl( res, env ) ;
res = internal::Rcpp_eval_impl( res, env ) ; // #nocov
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is important enough to test it?

typedef XPtr<class_Base> XP_Class ;
typedef Rcpp::XPtr<Rcpp::Module> XP ;
CppClass( SEXP x) : S4(x){};
CppClass( SEXP x) : S4(x){}; // #nocov
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should test instantiation from SEXP?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants