all: use github.com/deckarep/golang-set/v2 (generic set) #26159
Conversation
Jolly23
requested review from
fjl,
holiman,
karalabe and
rjl493456442
as code owners
|
This change is good, and we can apply it, but I want to do the upgrade to minimum Go version 1.18 as a separate step. |
|
go.mod Go requirement bump has been submitted in this PR: #26160 |
|
Good to know! |
Should I rewrite the fix this PR or you will handle the conflicts? |
|
Looks like the PR is still mergeable even though I updated the |
fjl
changed the title
| @@ -81,8 +81,8 @@ func (s *Server) ServeCodec(codec ServerCodec, options CodecOption) { | |||
| } | |||
|
|
|||
| // Add the codec to the set so it can be closed by Stop. | |||
| s.codecs.Add(codec) | |||
| defer s.codecs.Remove(codec) | |||
| s.codecs.Add(&codec) | |||
There was a problem hiding this comment.
This change looks like it changes the semantics, since it now adds the pointer to a codec instead of the codec struct. @fjl PTAL to double-check if this is ok
There was a problem hiding this comment.
I will fix this in a follow-up PR. Using a set here is weird.
There was a problem hiding this comment.
the generic mapset requests a comparable data type. The interface ServerCodec doesn't implement it. So I changed it to a pointer.
There was a problem hiding this comment.
Yes. This is one the cases where Go generics codifies something that couldn't be expressed in types before. Go allows interface values to be used as map keys, but will panic at runtime if the value contained in the interface is not comparable. With the generic set, this is not possible anymore. There is also no easy way to turn ServerCodec into something 'comparable'.
Have submitted #26180 to remove the pointer.
NEW Generics based implementation (requires Go 1.18 or higher) of mapset