Skip to content
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

* Changes to fix Subsetter's use of 'int' for indexing #920

Merged
merged 1 commit into from Nov 11, 2018

Conversation

Projects
None yet
4 participants
@nolanw123
Copy link

nolanw123 commented Nov 9, 2018

As discussed on the mailing list, this commit makes Subsetter use R_xlen_t for indexing/sizes. It also fixes a latent bug in indexing via Numeric's (the previous code created a temp IntegerVector of indices to use). Lastly, an exception is thrown in check_indices with a helpful warning message when attempting to index beyond 2^31 elements with an integral typed vector. All of these issues were basically only triggered in the case of indexing Vectors/Matrices with > 2^31 elements.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 9, 2018

Looks promising at a first glance. I will toss it at a reverse depends check once my hands are from the RcppArmadillo release I am working on right now. I am sure @kevinushey and @thirdwing will give it a good read too.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 11, 2018

Passed a full reverse depends check without issues, and nobody had anything else to object to (besides to the somewhat cosmetic issue raised by KK) so I think I'll merge this.

@eddelbuettel eddelbuettel merged commit d282bc1 into RcppCore:master Nov 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 11, 2018

I added the nicer message suggested by @thirdwing in 4f168e6.

@nolanw123

This comment has been minimized.

Copy link
Author

nolanw123 commented Nov 12, 2018

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Nov 13, 2018

Sorry! Was out of town and intentionally staying away from computers so didn't see this until now.

Post-hoc review is just a plain LGTM :-) Thanks for taking the time to file a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.