-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add integer overflow detection feature #78
Conversation
- added bool overflows<T>(csubstr) function
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
+ Coverage 93.96% 94.23% +0.26%
==========================================
Files 53 53
Lines 11449 11653 +204
==========================================
+ Hits 10758 10981 +223
+ Misses 691 672 -19
Continue to review full report at Codecov.
|
@fargies I think the And indeed sticking to Regarding the name, I was starting to write how I've added a commit (or two! hopefully the second one is good) addressing the compilation errors in test_charconv.cpp . As for the utilities, let's discuss those in a different issue or PR if you want to jump right in. Do have in mind that there is already a value-or-default method in NodeRef (but I can't recall the name ATM), which should be considered to avoid API cruft. Also, I will generally prefer methods in NodeRef rather than functions, as they are easier to find for users, whereas freestanding functions require explicit documentation which no one reads. The overflow is a different situation to this, as it is plainly type-related rather than data-related. ie, it is more c4core-scoped than ryml-scoped. |
Yay 🎉 Thanks for the fixes
I think it is |
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.
@fargies we're almost there; just address the two remarks, and I'll merge this in.
4f4cb45
to
6528440
Compare
@fargies the coverage test failure is from one of the coverage services (coveralls, I think I'm going to remove it). So all is good and from my end this is ready to merge. Is there anything else that you want to do? Let me know if I can merge. |
All good for me, can be merged 👍 |
Thanks! Great stuff, really appreciated. 🚀 |
Rework of #75
Overhead is 10-15% (when it was more 15-20% with previous implementation), I think overhead on u8 and i8 is more important just because there are more numbers with 3 digits spread on the domain (60%) so it has to compare bytes more often.
Also did not benchmark with hex/oct/bin encodings.
overflow_checked.json.txt
I did not add atoi_rc, it felt a bit too much, instead it may be interesting to implement a couple helpers at RapidYaml level (overriding
operator>>
), ex:What do you think ?
I can keep this as an extension to RapidYaml on our side if you'd prefer.