Drop support for go 1.18 #587
Conversation
fishy
requested review from
kylelemons,
pacejackson and
ghirsch-reddit
and removed request for
a team
fishy
force-pushed
the
atomic-pointer
branch
from
ca7ac11
to
44791dd
Compare
kafkabp/consumer.go
Outdated
| if c := kc.getConsumer(); c != nil { | ||
| if err := c.Close(); err != nil { | ||
| if c := kc.consumer.Load(); c != nil && *c != nil { | ||
| if err := (*c).Close(); err != nil { |
There was a problem hiding this comment.
🔕 The change here definitely feels worse. (*c)s and &c both? It looks wrong, at least.
As long as the type safety ensures that people don't mess it up, I'm probably fine with it, but it just doesn't seem great.
There was a problem hiding this comment.
it's kind of a compromise between excessive pointer (dereference) vs. type assertions to deal with any, as atomic.Pointer forces pointer even if the interface itself is already a pointer. the main problem is basically that there's not a way to say "pointer-like" in go's type system (e.g. make it happy to accept a pointer that implements an interface).
between the two I think type assertions are worse, but maybe that's just me :)
There was a problem hiding this comment.
I can keep the helper function to return a (possibly nil) interface instead, so the code here won't look as bad ¯\_(ツ)_/¯
tracing/tags.go
Outdated
| // never mutate the returned result, so skip copying is OK. | ||
| // | ||
| // But if any of those assumptions are broken in the future, we can not storing | ||
| // the atomic.Pointer[[]string] naively, and would need to either make a copy or |
There was a problem hiding this comment.
🔕 What you could also do is store an atomic.Pointer[string] and do a split whenver you need to access it. Splitting a string into a slice is quite fast, and in some cases you might even get escape analysis to notice the slice isn't leaked.
(only works if there is a safe way to use some delimiter without messing up the tags)
fishy
force-pushed
the
atomic-pointer
branch
from
fe2a4e1
to
10da24b
Compare
Use the new atomic package API introduced in go 1.19: * Replace atomic.Value with atomic.Pointer * Replace atomic.Add*/Store*/Load*/etc. with new Int*/Uint* atomic types * Except the atomic.Value used in filewatcher, as to take full advantage of that we would need to make breaking API changes to filewatcher
|
Made a slight word tweak on the comments because the original one has some grammar errors :) merging it. |
Use the new atomic package API introduced in go 1.19: