Codecov Report

Merging #78 (5d99fcd) into master (42bd976) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42bd976...5d99fcd. Read the comment docs.

@fargies I think the overflows<T>(s) is ready as-is, all the more so because its cost is rather low.

And indeed sticking to from_chars() is sufficient; it is a lot of work to add atoi_rc() and so on, and all it does is adding API complexity.

Regarding the name, I was starting to write how range_checked would be preferred, but then got your point about the utility suggestions and how range_checked is actually more appropriate for that scenario. I think you are right. So overflow_checked is how it goes.

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.

@fargies I think the overflows<T>(s) is ready as-is, all the more so because its cost is rather low.

Yay 🎉

Thanks for the fixes

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.

I think it is NodeRef::get_if, but you're right let's discuss this somewhere else 👍

@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. 🚀