refactor: Remove unused ParsePrechecks and ParseDouble #23156

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2110-refactorParseInt changing 4 files +41 −89

MarcoFalke commented at 3:57 PM on October 1, 2021: member
All of the ParsePrechecks are already done by ToIntegral, so remove them from ParseIntegral.

Also:
- Remove redundant {}. See #20457 (review)
- Add missing failing c-string test case
- Add missing failing test cases for non-int32_t integral types

refactor: Remove unused ParsePrechecks from ParseIntegral

Also:
* Remove redundant {} from return statement
* Add missing failing c-string test case and "-" and "+" strings
* Add missing failing test cases for non-int32_t integral types

fa3cd28535

MarcoFalke force-pushed on Oct 1, 2021
DrahtBot added the label Refactoring on Oct 1, 2021
DrahtBot added the label Utils/log/libs on Oct 1, 2021
fanquake requested review from practicalswift on Oct 2, 2021
fanquake requested review from laanwj on Oct 2, 2021
practicalswift commented at 8:41 AM on October 3, 2021: contributor

Concept ACK
practicalswift commented at 8:51 AM on October 3, 2021: contributor

@MarcoFalke Nice cleanup! I think we can get rid of the entire ParsePrechecks function now since the only remaining caller ParseDouble is unused (last use of ParseDouble removed in 78c312c983255e15fc274de2368a2ec13ce81cbf).
practicalswift commented at 6:23 PM on October 3, 2021: contributor

Note to reviewers: The string fuzzing harness (FUZZ=string src/test/fuzz/fuzz) can be used to try to test that ParseInt32 (and the other functions in that family) stays identical to the old behaviour (LegacyParseInt32) also after the removal of ParsePrechecks.
MarcoFalke renamed this:
~~refactor: Remove unused ParsePrechecks from ParseIntegral~~
refactor: Remove unused ParsePrechecks and ParseDouble
on Oct 4, 2021
Remove unused ParseDouble and ParsePrechecks fa9d72a794
MarcoFalke commented at 7:51 AM on October 4, 2021: member

@practicalswift Good find. Removed as well.
practicalswift approved
practicalswift commented at 7:52 AM on October 4, 2021: contributor

cr ACK fa9d72a7947d2cff541794e21e0040c3c1d43b32
MarcoFalke closed this on Oct 4, 2021
MarcoFalke reopened this on Oct 4, 2021

in src/test/util_tests.cpp:1480 in fa9d72a794

1473 | @@ -1474,6 +1474,35 @@ BOOST_AUTO_TEST_CASE(test_ParseInt32)
1474 |      BOOST_CHECK(!ParseInt32("32482348723847471234", nullptr));
1475 |  }
1476 |  
1477 | +template <typename T>
1478 | +static void RunToIntegralTests()
1479 | +{
1480 | +    BOOST_CHECK(!ToIntegral<T>(STRING_WITH_EMBEDDED_NULL_CHAR));

laanwj commented at 11:12 AM on October 4, 2021:

I like this test line.

laanwj commented at 11:13 AM on October 4, 2021: member

Code review ACK fa9d72a7947d2cff541794e21e0040c3c1d43b32, good find on ParseDouble not being used at all, and testing for behavior of embedded NULL characters is always a good thing.
MarcoFalke merged this on Oct 4, 2021
MarcoFalke closed this on Oct 4, 2021
sidhujag referenced this in commit 9f1da3bdbe on Oct 4, 2021
MarcoFalke deleted the branch on Oct 4, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

MarcoFalke

practicalswift

laanwj

Labels

Refactoring Utils/log/libs

Linked (view graph)

#20457 util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull}