Skip to content

refactor ToExponentAndMantissa to use StrFormat instead of std::stringstream#2138

Open
dmah42 wants to merge 3 commits intomainfrom
perf/refactor-string-util
Open

refactor ToExponentAndMantissa to use StrFormat instead of std::stringstream#2138
dmah42 wants to merge 3 commits intomainfrom
perf/refactor-string-util

Conversation

@dmah42
Copy link
Member

@dmah42 dmah42 commented Feb 27, 2026

No description provided.

Comment on lines 34 to 35
void ToExponentAndMantissa(double val, int precision, double one_k,
std::string* mantissa, int64_t* exponent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO this should be returning std::pair<std::string, int64_t>

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't hate output parameters, but they should at least be non-const references i think. but i'm also with returning a pair which does make sense for this API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(No strong opinion, consider it a suggestion)

}
}
mantissa_stream << val;
*mantissa += StrFormat("%g", val);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest extracting StrFormat("%g", x) into a lambda

Copy link
Member Author

Choose a reason for hiding this comment

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

that might make it less explicit/less readable. i'd rather not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My point is that all these calls are meant to have the same format specifier,
but now each call site provides it's own format string.
If not a lambda, i'd suggest to at least define a macro for "%d"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants