Vector::const_iterator is not analogous to Vector::iterator

[dcdillon]

I'm raising a new issue for this because in my investigations of #239 and #200 this has come up.

A small example of the issue is the code below. It is a const version of one of the Vector unit tests. It doesn't compile because the definition of const_iterator is not analogous to iterator.

#include <Rcpp.h>
#include <Rcpp/String.h>

using namespace Rcpp ;

// [[Rcpp::export]]
std::string character_const_iterator1( const CharacterVector letters ){
    std::string res ;
    CharacterVector::const_iterator first = letters.begin() ;
    CharacterVector::const_iterator last = letters.end() ;
    while( first != last ){
        res += *first ;
        ++first ;
    }
    return res ;
}

The reason it doesn't work is because of the definition of Vector::const_iterator. It is defined as follows:

template <int RTYPE>
struct r_vector_const_iterator {
        typedef const typename storage_type<RTYPE>::type* type ;
};

Which works fine for RTYPE = INTSXP, REALSXP, etc. but has no specialization (and I suspect there isn't a good one) for STRSXP which means that dereferencing a const_iterator for STRSXP yields a SEXP which of course can't be added to a std::string.

Vector::iterator is defined as follows:

template <int RTYPE> struct proxy_based_iterator{
        typedef ::Rcpp::internal::Proxy_Iterator< typename r_vector_proxy<RTYPE>::type > type ;
} ;

template<> struct r_vector_iterator<STRSXP> : proxy_based_iterator<STRSXP>{} ;

As a result, dereferencing a Vector::iterator yields a string_proxy which has all the appropriate operator overloads such that it can be added to a std::string.

Now the obvious fix is to have a proxy_based_iterator for const_iterator but this won't quite work because Proxy_Based_Iterator returns non-const references and pointers.

I would propose that a Const_Proxy_Based_Iterator be created that is identical in every way to Proxy_Based_Iterator except that operator*() returns a const & and operator->() returns a const *. Then Vector::const_iterator, at least for STRSXP can be defined as a Const_Proxy_Based_Iterator and then iterating over const Vector will be analogous to iterating over Vector.

As an aside, I've already written and tested this code and it passes all unit tests as well as the new ones added to test it. The trick will be in user packages that may have already worked around the insufficient definition of Vector<STRSXP>::const_iterator previously.

edit: misplaced a backtick that was improperly highlighting code.

[thirdwing]

I guess very few package are using the const_iterator.

But you should try https://github.com/RcppCore/rcpp-logs

[romainfrancois]

FWIW, Inspiration can be taken from Rcpp11/14 where this problem has been fixed.

[dcdillon]

@thirdwing - It is probably the case that NOBODY at all uses const_iterators on CharacterVectors.

@romainfrancois -

It seems that you defined a specialization of Vector to CharacterVector in Rcpp11.

It would seem to me that this is fine for a new project, but for an existing project it would be best to follow the pre-existing patterns (with, admittedly, a small addition). Then, if someone in the future decides to go through and handle things with specializations (as opposed to typedefs as they are now), this would get ported.

To do a specialization of CharacterVector now seems like it would institute a new pattern in the code base. This is a much bigger change than simply changing the definition of a const_iterator that is probably unused anyhow.

[dcdillon]

I took a deeper look at the Rcpp11 implementation and it has a problem. You can assign from a CharacterVector::const_iterator. So even if we wanted to do the specialization of the Vector class, it would still need a Const_Proxy_Iterator class (or something analogous).

[romainfrancois]

Can you log an issue in the Rcpp11 repo please

[dcdillon]

See master...dcdillon:Issue362 for code that passes all appropriate unit tests.

Turns out you don't actually need the Const_Proxy_Iterator class...Proxy_Iterator<const_proxy> is good enough.

[dcdillon]

At the request of @eddelbuettel I am summarizing the importance of this issue. There are currently 2 other issues that are at least somewhat dependent on this one.

In #239 it requires const_casts to work properly with CharacterVector/Matrix

in #200 it simply doesn't work for CharacterVector/Matrix without working const_iterators

As soon as this is resolved, I can have a PR for #200 ready to go.

[eddelbuettel]

@kevinushey @jjallaire when you have a moment in the next few days, please weigh in on this

[dcdillon]

Just wanted to poke this issue again.

The good news is, changing it should have zero effect on current code. As indicated before, dereferencing a const_iterator right now yields a SEXP. With the change, it will yield a const_proxy for the particular class. Fortunately, all of the const_proxy objects have cast operators to SEXP.

As a result, this change would a) make the iterators more useful, and b) not have any effect on current usage.

I will submit a PR with the code from master...dcdillon:Issue362 as soon as someone has a chance to look at it and confirm that I'm not crazy.

If/when this code is accepted, I have code already written that solves #200 and will be able to clean the const_casts out of #239.

[dcdillon]

I wanted to bring this issue up again since it will likely be a while before the next release. Can we get the vector iterator stuff in there so that the other issues can move forward? If not, I'll drop it, but it will really make some other things better in the future.

Thanks,

Dan

[eddelbuettel]

Actually I like where we are right now and think we could do 0.12.2 now-ishly, maybe next weekend, then mess around for 0.12.3.

But good to bump this. Not sure anybody has looked at it. Maybe time for a rcpp-core email ping too...

[dcdillon]

This one can be closed! I erroneously posted in #200.

[eddelbuettel]

And the commits were in PR #404.