Skip to content
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.

Sign up for GitHub

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

Jump to bottom

api: audit for Go 1.21 #60560

Closed
mknyszek opened this issue Jun 1, 2023 · 18 comments
Closed

api: audit for Go 1.21 #60560

mknyszek opened this issue Jun 1, 2023 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.21

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Jun 1, 2023 •
edited by ianlancetaylor

This is a tracking issue for doing an audit of API additions for Go 1.21 as of https://go.dev/cl/499981.

New API changes for Go 1.21

bytes

cmp

context

crypto/elliptic

crypto/rsa

crypto/tls

crypto/x509

debug/elf

encoding/binary

errors

flag

go/ast

go/build/constraint

go/build

go/token

html/template

io/fs

log/slog

maps

math/big

net/http

net

reflect

regexp

runtime

slices

strings

sync

syscall

testing

testing/slogtest

unicode
@mknyszek mknyszek added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels Jun 1, 2023
@mknyszek mknyszek added this to the Go1.21 milestone Jun 1, 2023
@ianlancetaylor
Copy link
Contributor

In #56984 it seems to me that the final accepted proposal was for

func (x *Int) Float64() (float64, Accuracy)

but what was actually implemented was

func (x *Int) ToFloat64() (float64, Accuracy)

I don't see any mention of ToFloat64 rather than Float64 in either #56984 or in https://go.dev/cl/453115.

CC @alandonovan

@ianlancetaylor
Copy link
Contributor

@jba When you have time, can you verify that all the log/slog and testing/slogtest exported names are the desired ones? Thanks.

@ianlancetaylor
Copy link
Contributor

@neild When you have time, can you verify that the various QUIC exported names in crypto/tls are the desired ones? Thanks.

@ianlancetaylor
Copy link
Contributor

I want to call out that the crypto/x509 API implemented for #53753 is not the API that the proposal explicitly accepted and doesn't seem to be mentioned in the proposal issue itself. I don't see any real problem here but I would like somebody to double check this. Thanks. CC @golang/security @rolandshoemaker @aarongable

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/500116 mentions this issue: math/big: rename Int.ToFloat64 to Float64

@rolandshoemaker
Copy link
Member

I think the naming discussion happened before the proposal was accepted, and the finalized API is listed in a subsequent comment (which I now notice was in fact made after the proposal was accepted itself), but the parent comment was never updated to reflect this.

I believe the naming in the follow-up (and what is the CL) is the better version of the proposed API, so while there is a minor mismatch in what was technically accepted and what was implemented, I think we ended up with what we actually want.

@ianlancetaylor
Copy link
Contributor

@rolandshoemaker Great, thanks.

gopherbot pushed a commit that referenced this issue Jun 2, 2023
@adonovan
math/big: rename Int.ToFloat64 to Float64
fdbc66d 
The "To" prefix was a relic of the first draft
that I failed to make consistent with the unprefixed
name used in the proposal. Fortunately iant spotted
it during the API audit.

Updates #56984
Updates #60560

Change-Id: Ifa6eeddf6dd5f0637c0568e383f9a4bef88b10f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/500116
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
@adonovan
Copy link
Member

adonovan commented Jun 2, 2023

What @rolandshoemaker said, with my apologies for creating more work for y'all.

@andig
Copy link
Contributor

andig commented Jun 4, 2023

pkg bytes, func ContainsFunc([]uint8, func(int32) bool) bool

Nitpicking: seems proposed and implemented was ContainsFunc([]uint8, func(rune) bool) bool, not sure if that matters.

@ianlancetaylor
Copy link
Contributor

@andig Thanks for the note. rune is an alias for int32, and the api tool resolves the alias. The actual source code says rune. So this is OK. Thanks.

@jba
Copy link
Contributor

jba commented Jun 5, 2023

@ianlancetaylor I've verified the slog symbols. At 5s/click, it will take me a while to check all the boxes. Is that necessary, or will this comment do?

@mknyszek
Copy link
Contributor Author

mknyszek commented Jun 5, 2023 •
edited

@jba I got you. For future reference, the checkbox state is changeable in the markdown. I just copied it into a text editor and did it from there. (EDIT: Annoying for sure, but at least there's some workaround! :P)

@jba
Copy link
Contributor

jba commented Jun 5, 2023

Thanks, @mknyszek!

@ianlancetaylor
Copy link
Contributor

Thanks.

@ianlancetaylor
Copy link
Contributor

@FiloSottile I see one difference between the crypto/tls changes that were committed and the API described at #60105: the committed code has a SessionState.EarlyData field that the issue doesn't seem to mention. Can you confirm that this is intended? Thanks.

@neild
Copy link
Contributor

neild commented Jun 6, 2023

The SessionState.EarlyData API was added in #60107.

@ianlancetaylor
Copy link
Contributor

@neild Thanks, totally missed that.

@ianlancetaylor
Copy link
Contributor

All APIs are now audited, so closing.

@ryan961 ryan961 mentioned this issue Jun 13, 2023
Open
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 14, 2023
@golang golang locked and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
9 participants
@neild @andig @mknyszek @dmitshur @ianlancetaylor @rolandshoemaker @adonovan @gopherbot @jba