-
Notifications
You must be signed in to change notification settings - Fork 402
Take advantage of new features in Go 1.23 #2775
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
Conversation
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…of O(N^2) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that it handles the legacy path as well. This will help future refactoring to use an iter.Seq. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that we don't have to allocate a single-use array Signed-off-by: Miloslav Trmač <mitr@redhat.com>
per (go fix ./...). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
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.
LGTM
func iterateAuthHeader(header http.Header) iter.Seq[challenge] { | ||
return func(yield func(challenge) bool) { | ||
for _, h := range header[http.CanonicalHeaderKey("WWW-Authenticate")] { | ||
v, p := parseValueAndParams(h) | ||
if v != "" { | ||
if !yield(challenge{Scheme: v, Parameters: p}) { | ||
return | ||
} | ||
} |
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.
Oh my, I like iterators but this new go syntax is something...
Anyway just some random comment has nothing to do with your change of course.
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.
Yes, it’s a bit mind-boggling that of all possible language features, they added this (which internally requires coroutines, see how iter.Pull*
are implemented).
OTOH, many other reasonable language features would ~require parameter and return value type inference, and I guess Go doesn’t want to go there.
layers[(len(m.FSLayers)-1)-i] = LayerInfo{ | ||
layers := make([]LayerInfo, 0, len(m.FSLayers)) | ||
for i, layer := range slices.Backward(m.FSLayers) { // NOTE: This includes empty layers (where m.History.V1Compatibility->ThrowAway) | ||
layers = append(layers, LayerInfo{ |
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.
Is this actually a bug fix as well? You commit message doesn't mention it but
types.BlobInfo{Digest: layer.BlobSum, Size: -1},
So since we walked forwards but set the element backwards the layer.BlobSum was actually not the one from the layer that was set. Not sure if this was by design like that or not. Of course the new change seems to make sense but does change behaviour I think
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 don’t think this is a behavior change, or I don’t understand what you mean.
- Previously: Read
m.FSLayers
andm.ExtractedV1Compatibility
, in order [0…N) (both the[i]
index and thelayer
value are consistent); write intolayers
, indexing, in order (N…0] - Now: Read Read
m.FSLayers
andm.ExtractedV1Compatibility
, in order (N…0] (both the[i]
index and thelayer
value are consistent); createlayers
by appending, in order [0…N)
AFAICS both are doing the same thing, only in the opposite order. Is that incorrect?
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.
Oh you are right, I got confused here sorry about that.
Somehow I read that as layer := layers[(len(m.FSLayers)-1)-i]
in the old version so I thought the layer was walked backwards while the m.ExtractedV1Compatibility[i]
was going forward.
See individual commit messages for details.
Cc: @Luap99