ci: bump golangci-lint to v2.9, fix some prealloc linter warnings#5118
ci: bump golangci-lint to v2.9, fix some prealloc linter warnings#5118kolyshkin wants to merge 2 commits intoopencontainers:mainfrom
Conversation
Add prealloc linter (for non-test files), fix its warnings when they make sense, and ignore when they don't (`runc features`). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
rata
left a comment
There was a problem hiding this comment.
You mention you skip this in runc features, but it doesn't seem to be here. Wasn't it needed in the end and the commit is outdated?
| var res []string | ||
| var res []string //nolint:prealloc | ||
| for k := range namespaceMapping { |
There was a problem hiding this comment.
using len(namespaceMapping) for the prealloc doesn't work here?
There was a problem hiding this comment.
This ^^^ is one the functions used by runc features only, so I chose to ignore rather than fix prealloc warnings.
There was a problem hiding this comment.
Oh, this one is it? Oh, okay. If we have the linter, I'd still do it... Not sure we want the linter, though
| var res []string | ||
| var res []string //nolint:prealloc | ||
| for k := range mountFlags { |
There was a problem hiding this comment.
This actually should look like this:
res := make([]string, 0, len(mountFlags) + len(mountPropagationMapping) + len(recAttrFlags) + len(extensionFLags)and this is kind of ugly, with no real performance benefit (as this is only used from runc features).
There was a problem hiding this comment.
The alternative to this is:
func KnownMountOptions() []string {
initMaps()
all := slices.Concat(
slices.Collect(maps.Keys(mountFlags)),
slices.Collect(maps.Keys(mountPropagationMapping)),
slices.Collect(maps.Keys(recAttrFlags)),
slices.Collect(maps.Keys(extensionFlags)),
)
slices.Sort(all)
return all
}which I don't like either as it creates some intermediate slices.
There was a problem hiding this comment.
I was unsure the noise/signal ratio of this linter was going to be useful. Clearly it can force us to write horrible code (like here). I'm not sure this linter is worth having.... :-/
There was a problem hiding this comment.
Removed (I agree it's more of a "recommendation" kind of linter). Kept some of the fixes which make sense.
There was a problem hiding this comment.
The more I look at this, the more unsure I am this is worth it. Linters should help us not review "trivial things" (like spaces, capitalization of variables, etc.) and make the code better.
If it ends up adding lot of "ignore this" everywhere, it is just too painful and we can remove it later. But I'm not convinced the value it adds. Also, our slices are not that big, I don't think it either has a big performance implication nor that the compiler in the future couldn't be smarter (our allocations are quite simple, most part of the same function).
Do you find yourself requesting this change on PRs? If so, then it is worth to have a linter.
If you like it, let's do add it :)
ci: bump golangci-lint to v2.9
Preallocate slices
These were found by the prealloc linter. While it does not make sense
to address all of prealloc warnings (or add it to the list of linters
we run in CI), some do make sense.