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

AttributeProxy::set() #946

Closed
romainfrancois opened this Issue Mar 12, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@romainfrancois
Copy link
Contributor

romainfrancois commented Mar 12, 2019

Should this method: https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/proxy/AttributeProxy.h#L52

        void set(SEXP x ){
            Rf_setAttrib( parent, attr_name, x ) ;
        }

rather be:

        void set(SEXP x){
            parent  = Rf_setAttrib( parent, attr_name, x ) ;
        }

and actually according to rchk it might as well be:

        void set(SEXP x){
            Shield<SEXP> safe_x(x);
            parent  = Rf_setAttrib( parent, attr_name, safe_x ) ;
        }

One of the insights from rchk when testing dplyr gives:

Function Rcpp::AttributeProxyPolicy<Rcpp::Vector<19, Rcpp::PreserveStorage> >::AttributeProxy::operator=(Rcpp::AttributeProxyPolicy<Rcpp::Vector<19, Rcpp::PreserveStorage> >::AttributeProxy const&)
  [UP] calling allocating function Rcpp::AttributeProxyPolicy<Rcpp::Vector<19, Rcpp::PreserveStorage> >::AttributeProxy::set(SEXPREC*) with argument allocated using Rcpp::AttributeProxyPolicy<Rcpp::Vector<19, Rcpp::PreserveStorage> >::AttributeProxy::get() const /opt/R-svn/packages/lib/Rcpp/include/Rcpp/proxy/AttributeProxy.h:34
@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 12, 2019

Not sure. Looking at a search for 'user:cran Rf_setAttrib' at GitHub I see some possible support for the Shield<> use. I don't think I have seen var = Rf_setAttrib(var, ...) call pattern. Where did you see this used or documented?

In general, we can certainly consider it. Easiest if you were to prepare a PR, test it on as many as the 1570-some reverse depends as you can, and thereby show that it doesn't make anything worse -- in that case we can of course consider such a carefully tested change.

Otherwise I am pretty much done with such tests and about to send Rcpp 1.0.1 to CRAN as indicated by email a few weeks ago.

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Mar 12, 2019

Right, I was confused about the parent =, turns out Rf_setAttrib() returns the value of the attribute :

Rcpp::cppFunction('SEXP setattr(SEXP x){ return Rf_setAttrib(x, Rf_install("foo"), Rf_ScalarInteger(2)); }' )
setattr(1:10)
#> [1] 2

The rchk thing still holds though, setAttrib potentially allocates, and so I believe it would make sense to protect x.

I see that some protection is given here:

template <typename CLASS>
template <typename T>
typename AttributeProxyPolicy<CLASS>::AttributeProxy&
AttributeProxyPolicy<CLASS>::AttributeProxy::operator=(const T& rhs) {
    set(Shield<SEXP>(wrap(rhs)));
    return *this;
}

but it seems it would be better in set(), for one thing it would take care of this method:

        AttributeProxy& operator=(const AttributeProxy& rhs){
            if( this != &rhs ) set( rhs.get() ) ;
            return *this ;
        }
@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 12, 2019

Yep -- I think tossing the Shield<> in is a no-brainer -- that really can't hurt. And Tomas just had me fix RInside for one of those 'might be allocating' that are easier to find in C code. He is pretty careful not to extrapolate too much from C++ in rchk reports. (If you're interested, running Gabor's container locally was easy and quick... I can give you more pointers).

Back to this. So basically your third variant minus the assigment back to parent ie

   void set(SEXP x){
            Shield<SEXP> safe_x(x);
            Rf_setAttrib(parent, attr_name, safe_x);
        }

Correct?

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Mar 12, 2019

I'm fine with outsourcing that to r-hub at this point. I've submitted a PR.

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Mar 12, 2019

using this form instead Rf_setAttrib( parent, attr_name, Shield<SEXP>(x) ) ;

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 12, 2019

Does r-hub now do reverse depends? Did I miss an announcement?

@romainfrancois

This comment has been minimized.

Copy link
Contributor Author

romainfrancois commented Mar 13, 2019

I meant, i’ll pass on the docker tutorial. I’m ok offloading my rchk needs to rhub at this point.

I’m not doing reverse depends checks here, or in any package i’m sending a pr to. I’ll add a test if I can think of one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.