These methods don't give us much, and removing them removes utilstrencodings's dependence on tinyformat
Inline i64tostr and itostr #13840
pull Empact wants to merge 1 commits into bitcoin:master from Empact:inline-itostr changing 6 files +14 −28-
Empact commented at 9:05 PM on August 1, 2018: member
- MarcoFalke added the label Refactoring on Aug 1, 2018
- Empact force-pushed on Aug 1, 2018
- DrahtBot cross-referenced this on Aug 1, 2018 from issue Include tinyformat as a subtree by Empact
-
DrahtBot commented at 10:05 PM on August 1, 2018: contributor
<!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:
- #13866 (utils: Use _wfopen and _wreopen on Windows by ken2812221)
- #13862 (utils: drop boost::interprocess::file_lock by ken2812221)
- #13846 (Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact)
- #13845 (Include tinyformat as a subtree by Empact)
- #13825 ([wallet] [Do not merge until v0.18] Kill accounts by jnewbery)
- #13787 (Test for #13426 by ken2812221)
- #13723 (PSBT key path cleanups by sipa)
- #13671 (Remove the boost/algorithm/string/case_conv.hpp dependency by 251Labs)
- #13426 ([bugfix] Fix encoding issue for Windows by ken2812221)
- #13168 (Thread names in logs and deadlock debug tools (take 2) by jamesob)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
- DrahtBot cross-referenced this on Aug 1, 2018 from issue Test for Windows encoding issue by ken2812221
- DrahtBot cross-referenced this on Aug 1, 2018 from issue PSBT key path cleanups by sipa
- DrahtBot cross-referenced this on Aug 1, 2018 from issue Remove the boost/algorithm/string/case_conv.hpp dependency by l2a5b1
- DrahtBot cross-referenced this on Aug 1, 2018 from issue [bugfix] Fix encoding issue for Windows by ken2812221
- DrahtBot cross-referenced this on Aug 2, 2018 from issue Include tinyformat as a subtree by Empact
-
practicalswift commented at 7:43 AM on August 2, 2018: contributor
utACK e00407dbbd13ccb8fd789811b4fee6871f737f9e
- DrahtBot cross-referenced this on Aug 2, 2018 from issue Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact
-
laanwj commented at 10:23 AM on August 2, 2018: member
I'm not sure this is a good idea. I kind of liked having separate, specialized functions for this. These currently happen to be implemented in terms of
strprintf, but this is not necessary and has relatively high overhead. (originally these were implemented using the appropriate C functions, but this was changed due to locale-dependence concerns)So tend toward NACK.
- Empact force-pushed on Aug 2, 2018
-
3f4c6187a8
Inline i64tostr and itostr
These methods don't give us much, and removing them removes utilstrencodings's dependence on tinyformat
- Empact force-pushed on Aug 2, 2018
- DrahtBot cross-referenced this on Aug 2, 2018 from issue [wallet] Kill accounts by jnewbery
-
Empact commented at 3:24 PM on August 2, 2018: member
I checked for cases in need of optimization and don't really see them. Here are some numbers which show about an order of magnitude of variance: http://www.zverovich.net/2013/09/07/integer-to-string-conversion-in-cplusplus.html fmt seems to have the fastest implementation according to these: http://www.zverovich.net/2013/09/07/integer-to-string-conversion-in-cplusplus.html https://github.com/fmtlib/fmt#speed-tests but I doubt it's worthwhile to bring that in.
Looking into that led me to inlining
WriteOrderPosand correcting the format string to be unsigned forccodeandnTransactionsUpdatedLast. - DrahtBot cross-referenced this on Aug 3, 2018 from issue utils: Use _wfopen and _wfreopen on Windows by ken2812221
- DrahtBot cross-referenced this on Aug 3, 2018 from issue utils: drop boost::interprocess::file_lock by ken2812221
-
donaloconnor commented at 9:54 PM on August 3, 2018: contributor
I'm tending towards a Nack for this too unfort.
Looking into that led me to inlining WriteOrderPos and correcting the format string to be unsigned for ccode and nTransactionsUpdatedLast.
When yous say inlining, you mean in the code but the function is inlined anyway so run time it won't have a performance impact. I like the functions since they document the code. It's easier to read :-)
-
Empact commented at 3:32 AM on August 4, 2018: member
Thanks for the look! Closing due to NACKs.
- Empact closed this on Aug 4, 2018
- bitcoin locked this on Sep 8, 2021