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

[Douglas Bates]

On Mon, May 10, 2010 at 8:32 AM, Romain Francois
<romain.francois at dbmail.com> wrote:
>
> 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
>
>
> --

As always, thanks very much for going above and beyond in responding
to questions.  I really appreciate your looking at this in such
detail.