[Rcpp-devel] Code review for xtensor R bindings

[Romain Francois]

Hi, 

Instead of doing that: 

const int vtype = traits::r_sexptype_traits<int>::rtype;
SEXP shape_sxp = Rf_allocVector(vtype, shape.size());

Perhaps your rxarray object could hold an IntegerVector. Also not quite sure why you use the first line at all, which does not depend on T, so vtype is always going to be INTSXP. 


Also, you should avoid using std::cout in https://github.com/QuantStack/xtensor-r/blob/master/src/rcpp_hello_xtensor.cpp#L14 <https://github.com/QuantStack/xtensor-r/blob/master/src/rcpp_hello_xtensor.cpp#L14>

You can use Rcpp::Rcout instead, or Rprintf. 

> And the memory is going to be freed again, at some point. And calling Rcpp_ReleaseObject(m_sexp) would also clear the shape_sxp "attribute", right?


The shape_sxp object you create here (and don’t protect)
https://github.com/QuantStack/xtensor-r/blob/master/inst/include/xtensor-r/rxarray.hpp#L152 <https://github.com/QuantStack/xtensor-r/blob/master/inst/include/xtensor-r/rxarray.hpp#L152>

Is probably duplicated here: 
https://github.com/QuantStack/xtensor-r/blob/master/inst/include/xtensor-r/rxarray.hpp#L160 <https://github.com/QuantStack/xtensor-r/blob/master/inst/include/xtensor-r/rxarray.hpp#L160>

As part of allocArray’s business: 
https://github.com/wch/r-source/blob/d60c742aa8acc764874db87e3c748e27986e1134/src/main/array.c#L259 <https://github.com/wch/r-source/blob/d60c742aa8acc764874db87e3c748e27986e1134/src/main/array.c#L259>

Romain



> Le 7 juin 2017 à 03:01, Wolf Vollprecht <w.vollprecht at gmail.com> a écrit :
> 
> Ok, I think I got the idea. 
> 
> In the R->xtensor bindings, we always work directly on memory allocated through R (either passed in from R or allocated from C++ code).
> 
> The review-relevant lines are really only ~10 lines: https://github.com/QuantStack/xtensor-r/blob/master/inst/include/xtensor-r/rxarray.hpp#L145 <https://github.com/QuantStack/xtensor-r/blob/master/inst/include/xtensor-r/rxarray.hpp#L145>
> 
>     const int vtype = traits::r_sexptype_traits<int>::rtype;
>     SEXP shape_sxp = Rf_allocVector(vtype, shape.size());
> 
>     int* r_shape = INTEGER(shape_sxp);
>         // set shape values ... 
>     m_sexp = Rf_allocArray(SXP, shape_sxp);
> 
> So afterwards, the idea would be to make a call to 
> 
> Rcpp_PreserveObject(m_sexp);
> 
> right? And if the destructor of the object is called (and the object is owned from C++) we would just call 
> 
> Rcpp_ReleaseObject(m_sexp);
> 
> And the memory is going to be freed again, at some point. And calling Rcpp_ReleaseObject(m_sexp) would also clear the shape_sxp "attribute", right?
> 
> Thanks for your reply! 
> 
> Wolf
> 
> 
> 2017-06-06 16:27 GMT-07:00 Dirk Eddelbuettel <edd at debian.org <mailto:edd at debian.org>>:
> 
> On 6 June 2017 at 17:06, Dirk Eddelbuettel wrote:
> | The general idea when working with R objects is that
> |
> |   -- on the way in from R to the C++ functions we construct object such that
> |      the existing memory (from R) is used; one example is how RcppArmadillo
> |      uses the 'advanced' constructors of Armadillo allowing to operate
> |      without copies; hence R managed memory is used while C++ functions are
> |      called; in general this side of the conversion is the templates as<>()
> |      converters;
> |
> |   -- on the way back we use the *alloc functions from the C-level API of R to
> |      construct objects that use memory from R's pool, once we return these
> |      objects to R as SEXP they are indistinguishable to R from its own; these
> |      are the wrap() functions (and I think we may make on occassion make "one
> |      final copy" at his level, but I'd have to double-check).
> 
> Actually, let me rephrase: "in most cases not involving native R types" do we
> make one copy at the return.
> 
> The general approach is iterator based, see internal/wrap.h. But there is a
> lot of template magic...
> 
> Dirk
> 
> | I haven't had a chance to look at what Wes is doing with Apache Arrow, and
> | what is happening with xtensor -- so thanks for getting the ball rolling.
> |
> | Dirk
> |
> | --
> | http://dirk.eddelbuettel.com <http://dirk.eddelbuettel.com/> | @eddelbuettel | edd at debian.org <mailto:edd at debian.org>
> | _______________________________________________
> | Rcpp-devel mailing list
> | Rcpp-devel at lists.r-forge.r-project.org <mailto:Rcpp-devel at lists.r-forge.r-project.org>
> | https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel <https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel>
> 
> --
> http://dirk.eddelbuettel.com <http://dirk.eddelbuettel.com/> | @eddelbuettel | edd at debian.org <mailto:edd at debian.org>
> 
> _______________________________________________
> Rcpp-devel mailing list
> Rcpp-devel at lists.r-forge.r-project.org
> https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.r-forge.r-project.org/pipermail/rcpp-devel/attachments/20170607/8c7d3b44/attachment-0001.html>