-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Add tests for Human Readable functionality #1632
Conversation
also fix an issue where the SI/IEC unit wasn't being correctly passed through.
b2cc7ad
to
301ab76
Compare
void AppendHumanReadable(int n, std::string* str) { | ||
std::stringstream ss; | ||
// Round down to the nearest SI prefix. | ||
ss << ToBinaryStringFullySpecified(n, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question. Why are we not sending Counter::kIs1000?
Or will we make this change afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wanted to get the barest minimal fixes for what's currently in the code before changing behaviour (aside from bug fixes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
@@ -43,8 +43,8 @@ void ToExponentAndMantissa(double val, double thresh, int precision, | |||
// Adjust threshold so that it never excludes things which can't be rendered | |||
// in 'precision' digits. | |||
const double adjusted_threshold = | |||
std::max(thresh, 1.0 / std::pow(10.0, precision)); | |||
const double big_threshold = adjusted_threshold * one_k; | |||
std::max(1.0, 1.0 / std::pow(10.0, precision)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, i like that thresh
goes away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me?
Thanks.
…nto inconsistent_suffixes_console_reporter_1009 (from- google#1632)
also fix an issue where the SI/IEC unit wasn't being correctly passed through.