util: Remove unused itostr #18449

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2003-utilNoIToStr changing 7 files +14 −26
  1. MarcoFalke commented at 1:02 pm on March 27, 2020: member
    Currently unused, but if someone really needed to use a helper with this functionality in the future, they could use ToString.
  2. util: Remove unused itostr fac96fff62
  3. MarcoFalke added the label Refactoring on Mar 27, 2020
  4. MarcoFalke added the label Utils/log/libs on Mar 27, 2020
  5. in src/sync.cpp:59 in fac96fff62 outdated 
    56@@ -57,7 +57,7 @@ struct CLockLocation {
57     {
58         return strprintf(
59             "%s %s:%s%s (in thread %s)",
    theStack commented at 1:17 pm on March 27, 2020:

    Now that the third parameter is of type int and not std::string anymore, the format string also has to be adapted?

    0            "%s %s:%d%s (in thread %s)",
    MarcoFalke commented at 1:32 pm on March 27, 2020:

    tinyformat.h is a type safe printf replacement library in a single C++ header file. If you’ve ever wanted printf("%s", s) to just work regardless of the type of s, tinyformat might be for you.

    https://github.com/c42f/tinyformat#a-minimal-type-safe-printf-replacement

    theStack commented at 1:53 pm on March 27, 2020:
    Ah, that’s nice and convenient. Is it smart though that the function has exactly the same name as the one from the standard library then (with not even a namespace in front)? As reader I’d hardly come to the idea that this would behave different than the standard implementation and be more forgiving.
    MarcoFalke commented at 1:55 pm on March 27, 2020:

    I don’t like the name either (I prefer tfm::format), but I think the name differs from the std lib one:

    0strprintf
1sprintf
    theStack commented at 2:03 pm on March 27, 2020:
    Oops – you are right! I missed the tr and read it indeed as sprintf. :walking_man:
  6. laanwj commented at 1:35 pm on March 27, 2020: member

    Nice.

    I guess we can replace i64tostr with ToString in the same way?

    (it’s only used in two real places)

  7. theStack commented at 2:03 pm on March 27, 2020: member
    Concept ACK
  8. util: Replace i64tostr with ToString faaf1cb5b9
  9. MarcoFalke force-pushed on Mar 27, 2020
  10. MarcoFalke commented at 3:50 pm on March 27, 2020: member

    I guess we can replace i64tostr with ToString in the same way?

    Done

  11. in src/test/fuzz/locale.cpp:86 in faaf1cb5b9 
    81@@ -82,10 +82,8 @@ void test_one_input(const std::vector<uint8_t>& buffer)
82     assert(atoi64c_without_locale == atoi64c_with_locale);
83     const int atoi_with_locale = atoi(random_string);
84     assert(atoi_without_locale == atoi_with_locale);
85-    const std::string i64tostr_with_locale = i64tostr(random_int64);
86-    assert(i64tostr_without_locale == i64tostr_with_locale);
87-    const std::string itostr_with_locale = itostr(random_int32);
88-    assert(itostr_without_locale == itostr_with_locale);
89+    const std::string tostring_with_locale = ToString(random_int64);
90+    assert(tostring_without_locale == tostring_with_locale);
    sipa commented at 4:13 pm on March 27, 2020:
    Isn’t this comparing a string with itself now?
    MarcoFalke commented at 4:17 pm on March 27, 2020: 
    0assert(tostring_without_locale
1    == tostring_with_locale);
    MarcoFalke commented at 4:20 pm on March 27, 2020:
    The fuzzer’s goal is to assert the function does not respond to changing the locale. So generating the same string with two different locales is needed. The check should assert that they are the same.
  12. practicalswift commented at 9:13 am on March 28, 2020: contributor

    Concept ACK: thanks for helping getting rid of accidental locale dependence!

    Scary valgrind error in Travis (unrelated I presume):

     0==28552== Invalid read of size 1
 1==28552==    at 0x193A13: CConnman::GetExtraOutboundCount() (net.cpp:1709)
 2==28552==    by 0x1CBCFD: PeerLogicValidation::EvictExtraOutboundPeers(long) (net_processing.cpp:3436)
 3==28552==    by 0x1CBF9A: PeerLogicValidation::CheckForStaleTipAndEvictPeers(Consensus::Params const&) (net_processing.cpp:3500)
 4==28552==    by 0x1D99DC: operator() (net_processing.cpp:1130)
 5==28552==    by 0x1D99DC: std::_Function_handler<void (), PeerLogicValidation::PeerLogicValidation(CConnman*, BanMan*, CScheduler&, CTxMemPool&)::$_0>::_M_invoke(std::_Any_data const&) (std_function.h:316)
 6==28552==    by 0x5C9974: operator() (std_function.h:706)
 7==28552==    by 0x5C9974: Repeat(CScheduler&, std::function<void ()>, std::chrono::duration<long, std::ratio<1l, 1000l> >) (scheduler.cpp:119)
 8==28552==    by 0x5C9782: operator() (scheduler.cpp:125)
 9==28552==    by 0x5C9782: std::_Function_handler<void (), CScheduler::scheduleEvery(std::function<void ()>, std::chrono::duration<long, std::ratio<1l, 1000l> >)::$_0>::_M_invoke(std::_Any_data const&) (std_function.h:316)
10==28552==    by 0x5C889D: operator() (std_function.h:706)
11==28552==    by 0x5C889D: CScheduler::serviceQueue() (scheduler.cpp:63)
12==28552==    by 0x169E79: operator() (init.cpp:1272)
13==28552==    by 0x169E79: std::_Function_handler<void (), AppInitMain(NodeContext&)::$_4>::_M_invoke(std::_Any_data const&) (std_function.h:316)
14==28552==    by 0x16D15E: operator() (std_function.h:706)
15==28552==    by 0x16D15E: void TraceThread<std::function<void ()> >(char const*, std::function<void ()>) (system.h:383)
16==28552==    by 0x17ADF4: void std::__invoke_impl<void, void (*&)(char const*, std::function<void ()>), char const*&, std::function<void ()>&>(std::__invoke_other, void (*&)(char const*, std::function<void ()>), char const*&, std::function<void ()>&) (invoke.h:60)
17==28552==    by 0x17AD7A: __invoke<void (*&)(const char *, std::function<void ()>), const char *&, std::function<void ()> &> (invoke.h:95)
18==28552==    by 0x17AD7A: __call<void, 0, 1> (functional:467)
19==28552==    by 0x17AD7A: operator()<, void> (functional:549)
20==28552==    by 0x17AD7A: boost::detail::thread_data<std::_Bind<void (*(char const*, std::function<void ()>))(char const*, std::function<void ()>)> >::run() (thread.hpp:116)
21==28552==    by 0x526CBCC: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.65.1)

    👀

  13. laanwj commented at 10:11 am on March 28, 2020: member
    ACK faaf1cb5b9a4c22b21757f7578833f908b79b867 Agree the valgrind error is scary but I don’t see any way it could be related (there are no changes to P2P code here).
  14. promag commented at 11:18 am on March 28, 2020: member
    Code review ACK faaf1cb5b9a4c22b21757f7578833f908b79b867.
  15. MarcoFalke commented at 1:27 pm on March 28, 2020: member

    appveyor run: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31768233 Travis run: https://travis-ci.org/github/bitcoin/bitcoin/builds/667726248

    Open-Close to re-run ci. See #15847 (comment)

  16. MarcoFalke closed this on Mar 28, 2020
  17. MarcoFalke reopened this on Mar 28, 2020
  18. MarcoFalke commented at 1:39 pm on March 28, 2020: member
    The issue valgrind found is a long-standing issue where the vector of nodes in Connman is not locked: See #18457 . Indeed unrelated to this pull.
  19. laanwj merged this on Mar 28, 2020
  20. laanwj closed this on Mar 28, 2020
  21. MarcoFalke deleted the branch on Mar 28, 2020
  22. Fabcien referenced this in commit ead7a4497d on Jan 13, 2021
  23. DrahtBot locked this on Feb 15, 2022


MarcoFalke theStack laanwj sipa practicalswift promag

Labels
Refactoring Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me