-
Notifications
You must be signed in to change notification settings - Fork 2
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
Merge pull request #22 from mrc-ide/mrc-1281 #22
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 16 16
Lines 1829 1787 -42
=====================================
- Hits 1829 1787 -42
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One purely narrative comment by the negative idx section, and one suggestiong for a link to the perhaps a link to the amazing reside blog when it's published.
I'll approve here, but leave you to merge when ready!
## dde 1.0.1 | ||
|
||
* Several memory errors fixed [#22](https://github.com/mrc-ide/dde/pull/22) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a link to the reside blog about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll retrospectively add it to avoid issues with the CRAN link checker
idx0 = min_size((t - t0) / (t1 - t0) / (n - 1), n - 1); | ||
if ((t0 - t) * (t1 - t) < 0) { | ||
idx0 = (t - t0) / (t1 - t0) / (n - 1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as we discussed: the values of t0, t1 and t (11, 12, 0.02) that caused the crash are perfectly reasonable; this part of the code should accept them gracefully, and then the code following this uses idx as a start point for searching the buffer.
Hence, zero is the best answer for idx to have after this bit of code, in the cases where it would previously have been negative.
As requested by CRAN on 1 January. Three fixes here - see reside-89 as reside-ic/reside-ic.github.io#34 for details.