-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fixed declaration dependencies and other lint issues in code base #1743
Conversation
replicas := []int32{} | ||
offlineReplicas := []int32{} | ||
var replicas []int32 | ||
var offlineReplicas []int32 |
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.
not sure about this one:
https://play.golang.org/p/vdAdcwi2i09
func main() {
a := []int32{}
var b []int32
fmt.Printf("a: %#v\n", a)
fmt.Printf("b: %#v\n", b)
}
result:
a: []int32{}
b: []int32(nil)
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.
hmm, yeah, that does changes semantics. but as far as I know, An empty slice can be represented by nil or an empty slice literal. The first approach is preferred as it does not lead to memory allocation. also tests are passing with changing to nil slice declaration. Let me know if that sounds good?
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.
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.
also tests are passing with changing to nil slice declaration
not sure if we are in a position to trust in the tests 😅
have you tried this branch in a running app?
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 asked some folks, and they mentioned that before the variable was initialized to an empty array, with your change the array is now nil.
However, append
will initialize the array internally
// The append built-in function appends elements to the end of a slice. If
// it has sufficient capacity, the destination is resliced to accommodate the
// new elements. If it does not, a new underlying array will be allocated.
https://play.golang.org/p/4EHaAlO5eIG
so, 🤞 it looks should work
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.
Go makes 'zero-values' do the expected thing. For slices, append
, len
, cap
, range
all behave well.
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'd be great if you can try this branch first on an existing app
Another lint/vet fix that's backward compatible.