Skip to content

Inline i64tostr and itostr#13840

Closed
Empact wants to merge 1 commit into
bitcoin:masterfrom
Empact:inline-itostr
Closed

Inline i64tostr and itostr#13840
Empact wants to merge 1 commit into
bitcoin:masterfrom
Empact:inline-itostr

Conversation

@Empact

@Empact Empact commented Aug 1, 2018

Copy link
Copy Markdown
Contributor

These methods don't give us much, and removing them removes utilstrencodings's
dependence on tinyformat

@DrahtBot

DrahtBot commented Aug 1, 2018

Copy link
Copy Markdown
Contributor
Note to reviewers: This pull request conflicts with the following ones:

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.

@practicalswift

Copy link
Copy Markdown
Contributor

utACK e00407dbbd13ccb8fd789811b4fee6871f737f9e

@laanwj

laanwj commented Aug 2, 2018

Copy link
Copy Markdown
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.

These methods don't give us much, and removing them removes utilstrencodings's
dependence on tinyformat
@Empact

Empact commented Aug 2, 2018

Copy link
Copy Markdown
Contributor Author

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 WriteOrderPos and correcting the format string to be unsigned for ccode and nTransactionsUpdatedLast.

@donaloconnor

Copy link
Copy Markdown
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

Empact commented Aug 4, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the look! Closing due to NACKs.

@Empact Empact closed this Aug 4, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants