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

[Romain Francois]

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).

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dim.diff
URL: <http://lists.r-forge.r-project.org/pipermail/rcpp-devel/attachments/20100510/d6f5b25b/attachment.asc>