Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork219
* Changes to fix Subsetter's use of 'int' for indexing#920
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
eddelbuettel commentedNov 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. |
Uh oh!
There was an error while loading.Please reload this page.
eddelbuettel commentedNov 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 commentedNov 11, 2018
I added the nicer message suggested by@thirdwing in4f168e6. |
nolanw123 commentedNov 12, 2018 via email
Thanks, looks like you got to it before I could :-).Cheers,Will …On Mon, Nov 12, 2018 at 12:30 AM Dirk Eddelbuettel ***@***.***> wrote: I added the nicer message suggested by@thirdwing <https://github.com/thirdwing> in4f168e6 <4f168e6> . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#920 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aqx9QVvCCbW03Ou-qQg3lj030w5wnZidks5uuEKsgaJpZM4YV-VW> . |
kevinushey commentedNov 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! |
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.