Skip to content
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

Jump to bottom

fmt,sync/atomic: fmt functions should allow integer verbs for atomic integers #54731

Open
bcmills opened this issue Aug 29, 2022 · 6 comments
Open

fmt,sync/atomic: fmt functions should allow integer verbs for atomic integers #54731

bcmills opened this issue Aug 29, 2022 · 6 comments
Assignees
@mknyszek
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Aug 29, 2022

What version of Go are you using (go version)?

go 1.19

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

https://go.dev/play/p/jHX7pcphAmV

What did you do?

Format an atomic.Int32 using the %d verb.

What did you expect to see?

Since an atomic.Int32 “is an atomic int32”, I expected that the fmt verbs that apply for int32 should generate the same representation when applied to an atomic.Int32.

Or, if the %d verb is not expected to work in the same way that it does for an int32, I expect the atomic package docs to note that detail.

What did you see instead?

Applying the %d verb to an atomic.Int32 prints it as a struct, not an integer:

{{} 42}

(CC @robpike @martisch)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 29, 2022
jamespwilliams pushed a commit to jamespwilliams/go that referenced this issue Aug 29, 2022
fmt: display contained values when formatting atomic booleans and int…
0480723 
…egers

* treat `atomic.{Int32,Int64,Uint32,Uint64}` values as integers when
  formatting, using the underlying integer value (by calling Load). This
  has the effect of, for example, formatting an atomic integer type
  containing 42 as "42", not "{{}, 42}").

* treat `atomic.Bool` values as booleans when formatting, using the
  contained boolean value (by calling Load). This has the effect of, for
  example, formatting an true atomic.Bool value containing as "true",
  not "{{}, 1}" or "{{} %!t(uint32=1)}" as before.

Fixes golang#54731
jamespwilliams added a commit to jamespwilliams/go that referenced this issue Aug 29, 2022
@jamespwilliams
fmt: display contained values when formatting atomic booleans and int…
a9ee026 
…egers

* treat `atomic.{Int32,Int64,Uint32,Uint64}` values as integers when
  formatting, using the underlying integer value (by calling Load). This
  has the effect of, for example, formatting an atomic integer type
  containing 42 as "42", not "{{}, 42}").

* treat `atomic.Bool` values as booleans when formatting, using the
  contained boolean value (by calling Load). This has the effect of, for
  example, formatting an true atomic.Bool value containing as "true",
  not "{{}, 1}" or "{{} %!t(uint32=1)}" as before.

Fixes golang#54731
@jamespwilliams jamespwilliams mentioned this issue Aug 29, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/426456 mentions this issue: fmt: display contained values when formatting atomic booleans and integers

@seankhliao seankhliao added NeedsFix The path to resolution is known, but the work has not been done. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Aug 29, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Aug 31, 2022 •
edited

I think that passing an atomic.Int32 (or any of the atomic types) by value is technically incorrect since it's noCopy, so the referenced code will fail go vet and the race detector (I hope... haha). However, I think this probably makes more sense for an *atomic.Int32 (is that weird to pass for %d?).

Also, I'm not certain that the documentation already allows the *atomic.Int32 case. Maybe we should also update the docs on the atomic type to not say "is an int32" in any form, since you can't use it by value. (As you suggested.)

If we want to support *atomic.Int32 specifically, maybe this should be a proposal? I'm not sure.

@mknyszek mknyszek self-assigned this Aug 31, 2022
@mknyszek mknyszek added this to the Backlog milestone Aug 31, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Aug 31, 2022

I think that passing an atomic.Int32 (or any of the atomic types) by value is technically incorrect since it's noCopy

Interesting — the noCopy check does fire if I run go vet manually, but doesn't appear to be included in the standard go test vet checks. 🤔

@mknyszek
Copy link
Contributor

That's odd, I feel like it was at some point because I've definitely had the noCopy check catch something in the runtime tests before, though perhaps I'm just misremembering.

@bcmills
Copy link
Contributor Author

bcmills commented Aug 31, 2022

Apparently not:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/test/test.go;l=601-627;drc=8a3d167f5b4b482a230cd9800df9969af60561af

@erikdubbelboer
Copy link
Contributor

Slightly related to #54582 I think.

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

@mknyszek mknyszek

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

fmt: display contained values when formatting atomic booleans and integers jamespwilliams/go
5 participants
@erikdubbelboer @mknyszek @bcmills @gopherbot @seankhliao