Skip to content

fmt: do not remove trailing zeros for %g and %G with #(sharp) flag #36588

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

Closed
wants to merge 15 commits into from

Conversation

yah01
Copy link
Contributor

@yah01 yah01 commented Jan 15, 2020

Fixes #36562

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jan 15, 2020
@yah01 yah01 changed the title fmt: do not remove trailing zeros for %g and %G fmt: do not remove trailing zeros for %g and %G with #(sharp) flag Jan 15, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: ca6628b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 1:

This change needs a test.


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 1:

(The issue has a good test set to use.)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: eb8c955) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 1:

Patch Set 1:

(The issue has a good test set to use.)

i added 3 tests from the issue


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: eaf2097) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 2:

Patch Set 2:

(1 comment)

thanks, done it now


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 3:

i realize there may be some bug, I will confirm it...


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 3:

Patch Set 3:

i realize there may be some bug, I will confirm it...

i will reply u after conforming,thanks!


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 586da2c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 4:

the bug occurs when print a decimal that ends with 0 before the decimal point. i fixed it and add a test for the case.


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 4: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=593b0105


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 4: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 6ca317c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 8:

The tests are correct, the new code is broken. In other words, the failures are catching a bug in the new code and should not be changed.

I tried these formats in C, which is the reference for this stuff, and it agrees with the existing tests:

% cat x.c #include <stdio.h> int main(void) { printf("%#g\n", 1e-10); printf("%#g\n", 0.); printf("%#12.5g\n", 0.); printf("%#12.5g\n", 100000.); printf("%#12.5g\n", 1.23e6); } % gcc x.c % a.out 1.00000e-10 0.00000 0.0000 1.0000e+05 1.2300e+06 %


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 6e1f2b6) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 9:

Patch Set 8:

The tests are correct, the new code is broken. In other words, the failures are catching a bug in the new code and should not be changed.

I tried these formats in C, which is the reference for this stuff, and it agrees with the existing tests:

% cat x.c #include <stdio.h> int main(void) { printf("%#g\n", 1e-10); printf("%#g\n", 0.); printf("%#12.5g\n", 0.); printf("%#12.5g\n", 100000.); printf("%#12.5g\n", 1.23e6); } % gcc x.c % a.out 1.00000e-10 0.00000 0.0000 1.0000e+05 1.2300e+06 %

thanks, i fix them all!


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 9: Code-Review+1

fixed


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 9:

This code makes more sense to me than the previous attempt, which troubled me (and turned out troublesome).

Please change the new variable name's prefix from "meet" to "saw" and tweak the comments.

This is likely to break things so it won't land until after the freeze is lifted for the next round of releases.


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 534b9f2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 10:

I changed the prefix of the variable and the comments.


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

}
}
if !hasDecimalPoint {
// decimal 0 should contribute once to digits.
if num[1] == '0' && len(num) == 2 {
Copy link
Contributor

@hasitpbhatt hasitpbhatt Jan 22, 2020

Choose a reason for hiding this comment

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

nit: It doesn't really matter, but I would have written it as

if len(num) == 2 && num[1] == '0' {
    digits--
}

Mostly because it asserts the length is greater than the index being accessed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your advice!

@gopherbot
Copy link
Contributor

This PR (HEAD: ed0eea1) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: 8021bb2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Seungbeom Kim:

Patch Set 12:

My understanding is that for the true zero (0) all digits are significant, regardless of their position with regards to the decimal point; e.g. in "0", "0.0", and "0.00". It's generally true that moving the decimal point (i.e. multiplying or dividing by 10^n) doesn't change the significance of the digits, and it holds for the zero as well. Therefore, "%#.4g" should print "0.000", not "0.0000".

This is also consistent with the behavior of C, which states (taken from ISO/IEC 9899:TC2):

e, E:
If the value is zero, the exponent is zero.

g, G:
Let P equal the precision if nonzero, 6 if the precision is omitted, or 1 if the precision is zero.
Then, if a conversion with style E would have an exponent of X:
— if P > X ≥ −4, the conversion is with style f (or F) and precision P − (X + 1).
— otherwise, the conversion is with style e (or E) and precision P − 1.

For the zero value, X is 0, so the conversion is with style f (or F) and precision P − 1, so "%#.4g" is equivalent to "%#.3f".


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 12:

Patch Set 12:

My understanding is that for the true zero (0) all digits are significant, regardless of their position with regards to the decimal point; e.g. in "0", "0.0", and "0.00". It's generally true that moving the decimal point (i.e. multiplying or dividing by 10^n) doesn't change the significance of the digits, and it holds for the zero as well. Therefore, "%#.4g" should print "0.000", not "0.0000".

This is also consistent with the behavior of C, which states (taken from ISO/IEC 9899:TC2):

e, E:
If the value is zero, the exponent is zero.

g, G:
Let P equal the precision if nonzero, 6 if the precision is omitted, or 1 if the precision is zero.
Then, if a conversion with style E would have an exponent of X:
— if P > X ≥ −4, the conversion is with style f (or F) and precision P − (X + 1).
— otherwise, the conversion is with style e (or E) and precision P − 1.

For the zero value, X is 0, so the conversion is with style f (or F) and precision P − 1, so "%#.4g" is equivalent to "%#.3f".

thanks! i understand it now


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 12: Run-TryBot+1

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 12:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=72e01641


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 12: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 12:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 12:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 405d51b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/215001 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from yah01:

Patch Set 12:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Feb 26, 2020
Fixes #36562

Change-Id: Id98ae9f7362cfb825b306c36649d505692d6d60e
GitHub-Last-Rev: 405d51b
GitHub-Pull-Request: #36588
Reviewed-on: https://go-review.googlesource.com/c/go/+/215001
Reviewed-by: Rob Pike <[email protected]>
@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 13: Code-Review+2


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rob Pike:

Patch Set 13:

RELNOTE=yes


Please don’t reply on this GitHub thread. Visit golang.org/cl/215001.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/215001 has been merged.

@gopherbot gopherbot closed this Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fmt: %#g behavior not consistent
4 participants