Skip to content

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

Merged
merged 11 commits into from
Mar 13, 2025
Merged

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Mar 12, 2025

See individual commit messages for details.

Cc: @Luap99

mtrmac added 11 commits March 12, 2025 20:16
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>
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mtrmac mtrmac merged commit 2c3e492 into containers:main Mar 13, 2025
10 checks passed
@mtrmac mtrmac deleted the go1.23 branch March 13, 2025 16:34
Comment on lines +64 to +72
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
}
}
Copy link
Member

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.

Copy link
Collaborator Author

@mtrmac mtrmac Mar 13, 2025

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{
Copy link
Member

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

Copy link
Collaborator Author

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 and m.ExtractedV1Compatibility, in order [0…N) (both the [i] index and the layer value are consistent); write into layers, indexing, in order (N…0]
  • Now: Read Read m.FSLayers and m.ExtractedV1Compatibility, in order (N…0] (both the [i] index and the layer value are consistent); create layers by appending, in order [0…N)

AFAICS both are doing the same thing, only in the opposite order. Is that incorrect?

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants