New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCpp containers lack `cbegin` / `cend` iterator pairs #741

Closed
SylvainCorlay opened this Issue Aug 13, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@SylvainCorlay

SylvainCorlay commented Aug 13, 2017

Methods cbegin and cend differ from begin and end in that they always return a const_iterator even when the object is not const.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 13, 2017

Member

We never had them in the API.

It's been a while since this came up, but I think part of the reason was than we could never guarantee const-ness of the underlying SEXP.

So what you write in QuantStack/xtensor-r#24 is somewhere between wrong and silly. Can't be our bug if we never had it. Your bug for assuming (without checking) we do.

But presume we can consider pull requests which implement / mocking / aliasing these.

Member

eddelbuettel commented Aug 13, 2017

We never had them in the API.

It's been a while since this came up, but I think part of the reason was than we could never guarantee const-ness of the underlying SEXP.

So what you write in QuantStack/xtensor-r#24 is somewhere between wrong and silly. Can't be our bug if we never had it. Your bug for assuming (without checking) we do.

But presume we can consider pull requests which implement / mocking / aliasing these.

@dcdillon

This comment has been minimized.

Show comment
Hide comment
@dcdillon

dcdillon Aug 13, 2017

Contributor

First, Rcpp is c++98 compatible for CRAN reasons. So cbegin() and cend() don't even exist.

Second, there are legacy constness problems. So to create a compliant cbegin() and cend() across the board is a bigger issue.

Now I'm not sure there is a way to write c++ code that inherently depends on cbegin() and cend() even in newer versions. So I would stick with the given interfaces.

Other than that, you could tackle the constness problems, which should be pretty straightforward in the vector types for numbers. It's a touch trickier for character vectors, but certainly not insurmountable.

In summary, the interfaces are more or less c++98ish. They do need to be updated (as well as constness corrected), but this is not a barrier to usage. Constness would be a good project. Let's get it rolling!

Contributor

dcdillon commented Aug 13, 2017

First, Rcpp is c++98 compatible for CRAN reasons. So cbegin() and cend() don't even exist.

Second, there are legacy constness problems. So to create a compliant cbegin() and cend() across the board is a bigger issue.

Now I'm not sure there is a way to write c++ code that inherently depends on cbegin() and cend() even in newer versions. So I would stick with the given interfaces.

Other than that, you could tackle the constness problems, which should be pretty straightforward in the vector types for numbers. It's a touch trickier for character vectors, but certainly not insurmountable.

In summary, the interfaces are more or less c++98ish. They do need to be updated (as well as constness corrected), but this is not a barrier to usage. Constness would be a good project. Let's get it rolling!

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 13, 2017

So what you write in QuantStack/xtensor-r#24 is somewhere between wrong and silly.

Wow... On a technical standpoint, xtensor assumes that the underlying shape type implements the standard methods of STL containers. Maybe that was wrong and silly.

Can't be our bug if we never had it.

My intent when opening an issue in the tracker for a project is not to throw blame on anyone but help improving the project as I am using it.

SylvainCorlay commented Aug 13, 2017

So what you write in QuantStack/xtensor-r#24 is somewhere between wrong and silly.

Wow... On a technical standpoint, xtensor assumes that the underlying shape type implements the standard methods of STL containers. Maybe that was wrong and silly.

Can't be our bug if we never had it.

My intent when opening an issue in the tracker for a project is not to throw blame on anyone but help improving the project as I am using it.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Aug 13, 2017

Member

Absolutely, and we welcome that. But Rcpp interfaces R, and some things are simply different from what you may have seen for Python and other languages. We simply aim to clarify where your assumptions may have mislead you.

As @dcdillon says, this would be a good project to have, and not a trivial one to get right. Maye we should do some talking and sketching.

As for the standards methods of STL containers, we do have begin, end and much more. We also warn that certain access functions are there, yet should possibly not be used (because underlying storage is always SEXP based making some ops rather costly).

Member

eddelbuettel commented Aug 13, 2017

Absolutely, and we welcome that. But Rcpp interfaces R, and some things are simply different from what you may have seen for Python and other languages. We simply aim to clarify where your assumptions may have mislead you.

As @dcdillon says, this would be a good project to have, and not a trivial one to get right. Maye we should do some talking and sketching.

As for the standards methods of STL containers, we do have begin, end and much more. We also warn that certain access functions are there, yet should possibly not be used (because underlying storage is always SEXP based making some ops rather costly).

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Aug 13, 2017

First, Rcpp is c++98 compatible for CRAN reasons. So cbegin() and cend() don't even exist.

Second, there are legacy constness problems. So to create a compliant cbegin() and cend() across the board is a bigger issue.

I am aware of the novelty of cbegin / cend. Although adding them would not be a problem from a C++98 perspective.

Constness would be a good project. Let's get it rolling!

Yes, as I was posting the issue, I was looking at what it would take to make the Rcpp sequences be const-correct.

SylvainCorlay commented Aug 13, 2017

First, Rcpp is c++98 compatible for CRAN reasons. So cbegin() and cend() don't even exist.

Second, there are legacy constness problems. So to create a compliant cbegin() and cend() across the board is a bigger issue.

I am aware of the novelty of cbegin / cend. Although adding them would not be a problem from a C++98 perspective.

Constness would be a good project. Let's get it rolling!

Yes, as I was posting the issue, I was looking at what it would take to make the Rcpp sequences be const-correct.

@dcdillon

This comment has been minimized.

Show comment
Hide comment
@dcdillon

dcdillon Aug 13, 2017

Contributor

I'm happy to help. I've already solved the char vector problem. I just don't have time to be the point person for constness currently.

Contributor

dcdillon commented Aug 13, 2017

I'm happy to help. I've already solved the char vector problem. I just don't have time to be the point person for constness currently.

@dcdillon

This comment has been minimized.

Show comment
Hide comment
@dcdillon

dcdillon Aug 13, 2017

Contributor

To @eddelbuettel point, there are also std algorithm problems...specifically with char vectors. If we could launch a project to fix this, that'd be good for everyone.

Contributor

dcdillon commented Aug 13, 2017

To @eddelbuettel point, there are also std algorithm problems...specifically with char vectors. If we could launch a project to fix this, that'd be good for everyone.

@dcdillon

This comment has been minimized.

Show comment
Hide comment
@dcdillon

dcdillon Aug 13, 2017

Contributor

Iirc string_proxy is not move constructible. This breaks any std algorithm that reorders a container.

While this isn't necessary for numerical computation, we should just do it all at once and get it right imho.

Contributor

dcdillon commented Aug 13, 2017

Iirc string_proxy is not move constructible. This breaks any std algorithm that reorders a container.

While this isn't necessary for numerical computation, we should just do it all at once and get it right imho.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Aug 13, 2017

Contributor

@dcdillon: I'll throw some time behind this. Do you have a general game plan? Or should I reference the char vector work you've done?

Contributor

coatless commented Aug 13, 2017

@dcdillon: I'll throw some time behind this. Do you have a general game plan? Or should I reference the char vector work you've done?

@dcdillon

This comment has been minimized.

Show comment
Hide comment
@dcdillon

dcdillon Aug 13, 2017

Contributor

It's not anywhere you can see. It lives in infamy on my local computer. If you can spearhead, I can help review and do the string stuff.

Contributor

dcdillon commented Aug 13, 2017

It's not anywhere you can see. It lives in infamy on my local computer. If you can spearhead, I can help review and do the string stuff.

@dcdillon

This comment has been minimized.

Show comment
Hide comment
@dcdillon

dcdillon Aug 16, 2017

Contributor

We have started work on this. It's in the preliminary stages, but I think we have things in a passable state for Vectors of primitives. The other issues with more complex types have not been addressed yet. The code is available at https://github.com/coatless/Rcpp in the feature/const-iter branch. Any help testing would be appreciated.

Contributor

dcdillon commented Aug 16, 2017

We have started work on this. It's in the preliminary stages, but I think we have things in a passable state for Vectors of primitives. The other issues with more complex types have not been addressed yet. The code is available at https://github.com/coatless/Rcpp in the feature/const-iter branch. Any help testing would be appreciated.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless
Contributor

coatless commented Aug 16, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jan 10, 2018

Member

This got merged a release ago and we forgot to close this.

Member

eddelbuettel commented Jan 10, 2018

This got merged a release ago and we forgot to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment