feat: improve slice usage from pooled objects #160
Conversation
| @@ -45,18 +45,18 @@ func HTTPHeadersToStructPb(h http.Header) *structpb.Struct { | |||
| return nil | |||
| } | |||
|
|
|||
| func HTTPHeadersToModelpb(h http.Header) []*modelpb.HTTPHeader { | |||
| func HTTPHeadersToModelpb(h http.Header, out []*modelpb.HTTPHeader) []*modelpb.HTTPHeader { | |||
There was a problem hiding this comment.
This doesn't seem right, we're passing out and also returning out?
There was a problem hiding this comment.
If out is not big enough we might append to it, so we need to return a slice because they might not match
There was a problem hiding this comment.
I find this API a bit confusing, I think it'd be best to do one or the other. If we want to accept the out as an argument, then we can pass out *[]*modelpb.HTTPHeader
There was a problem hiding this comment.
I guess this signature looks similar to s = append(s, struct{}), so that's an argument for this pattern. But we'll have to remember to use the return value.
There was a problem hiding this comment.
But for conversion purposes like this specific one, I think @marclop 's comment makes sense. It will be clearer.
There was a problem hiding this comment.
I'm a bit conflicted. This method was inspired by the slices package in the stdlib (https://pkg.go.dev/slices)
There are several methods/API there that returns the result (modified slice) despite changing the argument (Compact, Grow, Replace, Clip, etc.). I assume they might in turn have copied the signature of append.
I think this is a common pattern in go ecosystem but I don't feel strongly about it.
There was a problem hiding this comment.
Thanks for chiming in. I was unaware of the widespread apdoption of this pattern in the current Go stdlib. I still think it's clearer and simpler to accept a single argument. If we leave it as is, I would prefer if we documented the behaviour since there are currently no godocs for these functions.
| func ToKv(m map[string]any) []*modelpb.KeyValue { | ||
| nm := normalizeMap(m) | ||
| if len(nm) == 0 { | ||
| func ToKv(m map[string]any, out []*modelpb.KeyValue) []*modelpb.KeyValue { |
Followup to #158
Part 3 of #152
Objects retrieved from the vtpool have preallocated empty slices. We should reuse them instead of creating new one.
Related to #130