-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
log: use atomic types #27763
log: use atomic types #27763
Conversation
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.
LGTM
log/format.go
Outdated
} | ||
} | ||
|
||
// locationEnabled is an atomic flag controlling whether the terminal formatter | ||
// should append the log locations too when printing entries. | ||
var locationEnabled uint32 | ||
var locationEnabled atomic.Uint32 |
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.
This should be an atomic.Bool
, really. It's a small enough change that we might just as well fix it in this PR while we're at it, IMO,
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.
Thanks, done
log/handler_glog.go
Outdated
override uint32 // Flag whether overrides are used, atomically accessible | ||
backtrace uint32 // Flag whether backtrace location is set | ||
level atomic.Uint32 // Current log level, atomically accessible | ||
override atomic.Uint32 // Flag whether overrides are used, atomically accessible |
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.
This could also be a boolean. Instead of setting it to len(filter)
, we would set it to len(filter) != 0
log/handler_glog.go
Outdated
return h.origin.Log(r) | ||
} | ||
// If no local overrides are present, fast track skipping | ||
if atomic.LoadUint32(&h.override) == 0 { | ||
if h.override.Load() { |
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.
It needs to be either
h.override.Store(len(filter) != 0)
and check if !h.override.Load() {
or
h.override.Store(len(filter) == 0)
and check if h.override.Load() {
log/handler_glog.go
Outdated
backtrace uint32 // Flag whether backtrace location is set | ||
level atomic.Uint32 // Current log level, atomically accessible | ||
override atomic.Bool // Flag whether overrides are used, atomically accessible | ||
backtrace atomic.Uint32 // Flag whether backtrace location is set |
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.
hm, backtrace also looks very boolean, in it's usage, to me :)
e0b71c0
to
0d9e6fd
Compare
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.
LGTM
Co-authored-by: Felix Lange <[email protected]>
This reverts commit 818db9b.
This reverts commit 818db9b.
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
No description provided.