Skip to content

util: Remove unused itostr #18449

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 2 commits into from
Mar 28, 2020
Merged

util: Remove unused itostr #18449

laanwj merged 2 commits into from
Mar 28, 2020

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 27, 2020

Currently unused, but if someone really needed to use a helper with this functionality in the future, they could use ToString.

theStack
theStack reviewed Mar 27, 2020
View reviewed changes
@laanwj
Copy link
Member

laanwj commented Mar 27, 2020 •
edited
Loading

Nice.

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

(it's only used in two real places)

@theStack
Copy link
Contributor

theStack commented Mar 27, 2020

Concept ACK

@maflcko maflcko force-pushed the 2003-utilNoIToStr branch from fa7f612 to faaf1cb Compare March 27, 2020 14:14
@maflcko
Copy link
Member Author

maflcko commented Mar 27, 2020

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

Done

@practicalswift practicalswift mentioned this pull request Mar 27, 2020
Closed
sipa
sipa reviewed Mar 27, 2020
View reviewed changes
src/test/fuzz/locale.cpp
const std::string itostr_with_locale = itostr(random_int32);
assert(itostr_without_locale == itostr_with_locale);
const std::string tostring_with_locale = ToString(random_int64);
assert(tostring_without_locale == tostring_with_locale);
Copy link
Member

@sipa sipa Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this comparing a string with itself now?

Copy link
Member Author

@maflcko maflcko Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

assert(tostring_without_locale
    == tostring_with_locale);

Copy link
Member Author

@maflcko maflcko Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@practicalswift
Copy link
Contributor

practicalswift commented Mar 28, 2020 •
edited
Loading

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

Scary valgrind error in Travis (unrelated I presume):

==28552== Invalid read of size 1
==28552==    at 0x193A13: CConnman::GetExtraOutboundCount() (net.cpp:1709)
==28552==    by 0x1CBCFD: PeerLogicValidation::EvictExtraOutboundPeers(long) (net_processing.cpp:3436)
==28552==    by 0x1CBF9A: PeerLogicValidation::CheckForStaleTipAndEvictPeers(Consensus::Params const&) (net_processing.cpp:3500)
==28552==    by 0x1D99DC: operator() (net_processing.cpp:1130)
==28552==    by 0x1D99DC: std::_Function_handler<void (), PeerLogicValidation::PeerLogicValidation(CConnman*, BanMan*, CScheduler&, CTxMemPool&)::$_0>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==28552==    by 0x5C9974: operator() (std_function.h:706)
==28552==    by 0x5C9974: Repeat(CScheduler&, std::function<void ()>, std::chrono::duration<long, std::ratio<1l, 1000l> >) (scheduler.cpp:119)
==28552==    by 0x5C9782: operator() (scheduler.cpp:125)
==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)
==28552==    by 0x5C889D: operator() (std_function.h:706)
==28552==    by 0x5C889D: CScheduler::serviceQueue() (scheduler.cpp:63)
==28552==    by 0x169E79: operator() (init.cpp:1272)
==28552==    by 0x169E79: std::_Function_handler<void (), AppInitMain(NodeContext&)::$_4>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==28552==    by 0x16D15E: operator() (std_function.h:706)
==28552==    by 0x16D15E: void TraceThread<std::function<void ()> >(char const*, std::function<void ()>) (system.h:383)
==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)
==28552==    by 0x17AD7A: __invoke<void (*&)(const char *, std::function<void ()>), const char *&, std::function<void ()> &> (invoke.h:95)
==28552==    by 0x17AD7A: __call<void, 0, 1> (functional:467)
==28552==    by 0x17AD7A: operator()<, void> (functional:549)
==28552==    by 0x17AD7A: boost::detail::thread_data<std::_Bind<void (*(char const*, std::function<void ()>))(char const*, std::function<void ()>)> >::run() (thread.hpp:116)
==28552==    by 0x526CBCC: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.65.1)

👀

@laanwj
Copy link
Member

laanwj commented Mar 28, 2020 •
edited
Loading

ACK faaf1cb
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).

@promag
Copy link
Contributor

promag commented Mar 28, 2020

Code review ACK faaf1cb.

@maflcko
Copy link
Member Author

maflcko commented Mar 28, 2020

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)

@maflcko maflcko closed this Mar 28, 2020
@maflcko maflcko reopened this Mar 28, 2020
@maflcko
Copy link
Member Author

maflcko commented Mar 28, 2020

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.

@laanwj laanwj merged commit 1668c80 into bitcoin:master Mar 28, 2020
@maflcko maflcko deleted the 2003-utilNoIToStr branch March 28, 2020 19:11
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 13, 2021
@PiRK
util: Remove unused itostr
ead7a44 
Summary:
This is a backport of Core [[bitcoin/bitcoin#18449 | PR18449]] [1/2]
bitcoin/bitcoin@fac96ff

Test Plan:
```
ninja all check-all
grep -R itostr ..
```

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8890
@barton2526 barton2526 mentioned this pull request Aug 13, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa left review comments

+1 more reviewer

@theStack theStack theStack left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @laanwj @theStack @practicalswift @promag @sipa