refactor: Remove unused ParsePrechecks and ParseDouble #23156

1. 
   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
2. 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
3. MarcoFalke force-pushed on Oct 1, 2021
4. DrahtBot added the label Refactoring on Oct 1, 2021
5. DrahtBot added the label Utils/log/libs on Oct 1, 2021
6. fanquake requested review from practicalswift on Oct 2, 2021
7. fanquake requested review from laanwj on Oct 2, 2021
8. 
   practicalswift commented at 8:41 am on October 3, 2021: contributor

   Concept ACK
9. 
   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).
10. 
    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.
11. MarcoFalke renamed this:
    refactor: Remove unused ParsePrechecks from ParseIntegral
    refactor: Remove unused ParsePrechecks and ParseDouble
    on Oct 4, 2021
12. Remove unused ParseDouble and ParsePrechecks fa9d72a794
13. 
    MarcoFalke commented at 7:51 am on October 4, 2021: member

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

    cr ACK fa9d72a7947d2cff541794e21e0040c3c1d43b32
16. MarcoFalke closed this on Oct 4, 2021

    ---

17. MarcoFalke reopened this on Oct 4, 2021

    ---

18. 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.
19. 
    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.
20. MarcoFalke merged this on Oct 4, 2021
21. MarcoFalke closed this on Oct 4, 2021

    ---

22. sidhujag referenced this in commit 9f1da3bdbe on Oct 4, 2021
23. MarcoFalke deleted the branch on Oct 4, 2021
24. DrahtBot locked this on Oct 30, 2022

Contributors
MarcoFalke practicalswift laanwj

Labels
Refactoring Utils/log/libs