[Rcpp-devel] Rcpp::Dimension initialized from an SEXP

[Romain Francois]

Le 10/05/10 15:06, Romain Francois a écrit :
> Le 10/05/10 14:13, Romain Francois a écrit :
>>
>> Le 08/05/10 16:21, Douglas Bates a écrit :
>>> The enclosed section of code using Rcpp::Dimension fails to compile
>>> because of the const qualifiers for the simple::nrow and simple::ncol
>>> method functions. If you omit those const qualifiers then it will
>>> compile and behave as desired. Of course, I would prefer to have
>>> those method functions use the const qualifier. Does anyone have
>>> suggestions on how to modify this code so I can use the const
>>> qualifiers?
>>>
>>> My guess is that this is related to the Rcpp::Dimension::operator[]()
>>> returning an int& and not an int so somehow there is a delayed
>>> evaluation going on - but I haven't learned enough C++ to be able to
>>> decide exactly where the problem originates.
>>
>> Hmm. The compiler error is really weird. It is not related to an
>> underlying SEXP because there is no underlying SEXP. Rcpp::Dimension
>> stores its data in a std::vector<int>
>>
>> If I simplfy the code to this :
>>
>> library(Rcpp)
>> cdef <- '
>> class simple {
>> Rcpp::Dimension dd;
>> public:
>> simple() : dd(1, 3) {}
>> int nrow() const { return dd[0] ; }
>> };
>> '
>> cpp <- '
>> simple ss;
>> return wrap( ss.nrow() ) ;
>> '
>> ff <- cppfunction(signature(), cpp, includes = cdef)
>> ff( )
>>
>>
>> I get this compiler error:
>>
>> Le chargement a nécessité le package : inline
>> Le chargement a nécessité le package : methods
>> file10d63af1.cpp: In member function ‘int simple::nrow() const’:
>> file10d63af1.cpp:8: error: invalid use of incomplete type ‘struct
>> SEXPREC’
>> /Library/Frameworks/R.framework/Resources/include/Rinternals.h:333:
>> error: forward declaration of ‘struct SEXPREC’
>> file10d63af1.cpp:8: error: cannot convert ‘SEXPREC’ to ‘int’ in return
>> make: *** [file10d63af1.o] Error 1
>>
>>
>> which is __really__ weird... (time passes)... I think it comes from the
>> operator SEXP. The compiler decided, for some reason to convert the
>> Rcpp::Dimension to a SEXP using operator SEXP, then add 0 and
>> dereference:
>>
>> so dd[0] becomes *( dd + 0 ) which becomes *( (SEXP)dd + 0 ) and I guess
>> this is where it gets unhappy.
>>
>>
>> If I change the code to explicitely call the operator[]:
>>
>> int nrow() const { return dd.operator[](0) ; }
>>
>> I then get a much more reasonnable compiler error:
>>
>> file10d63af1.cpp: In member function ‘int simple::nrow() const’:
>> file10d63af1.cpp:8: error: passing ‘const Rcpp::Dimension’ as ‘this’
>> argument of ‘int& Rcpp::Dimension::operator[](int)’ discards qualifiers
>> make: *** [file10d63af1.o] Error 1
>>
>>
>> I am not sure why the compiler does not want to call the operator[], so
>> there is probably something we miss in Rcpp::Dimension.
>>
>> Also, Rcpp::Dimension objects are often used temporarily (in matrix
>> constructors, etc ...) so maybe we don't need them to return references
>> and can return int instead. If you want to play with changing semantics
>> of Rcpp::Dimension so that it returns "int" instead of "int&", that's
>> fine by me. at the moment I don't think we do anything like:
>>
>> Rcpp::Dimension x(3,3) ;
>> x[0] = 4 ;
>>
>> so we don't need references here.
>>
>>
>> We might still need two versions of operator[] to deal with the
>> eccentric decision from the compiler.
>>
>> Romain
>
> I get it now. Your code compiles after the attached patch is applied ( I
> can't do it right now because r-forge is full and won't let me commit).

Seems to be back up now. So I've commited it.

> The compiler is not eccentric, it just knows the rule better than we do.
> Since the operator[] is not const, the compiler does not want it when
> doing dd[0], then it looks at alternative and has the idea to cast the
> Dimension to a SEXP and dereference that after.
>
> The patch adds a const operator[]:
>
> typedef std::vector<int>::reference reference ;
> typedef std::vector<int>::const_reference const_reference ;
> reference operator[](int i) throw(std::range_error) ;
> const_reference operator[](int i) const throw(std::range_error) ;
>
> which suits your code, but keeps the reference semantics so that we
> don't break any code that would use them.
>
> I think there are more examples like this in one of Scott Meyers's books
> (effective c++, ...), about how we sometimes need const and non const
> versions of operator[].
>
> Romain


-- 
Romain Francois
Professional R Enthusiast
+33(0) 6 28 91 30 30
http://romainfrancois.blog.free.fr
|- http://bit.ly/9aKDM9 : embed images in Rd documents
|- http://tr.im/OIXN : raster images and RImageJ
|- http://tr.im/OcQe : Rcpp 0.7.7