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.

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

proposal: all: have a policy for sweeping updates #67510

Open
robpike opened this issue May 18, 2024 · 8 comments
Open

proposal: all: have a policy for sweeping updates #67510

robpike opened this issue May 18, 2024 · 8 comments
Labels
Milestone

Comments

@robpike
Copy link
Contributor

robpike commented May 18, 2024

Proposal Details

See https://go-review.googlesource.com/c/go/+/586616 for example. There's nothing wrong with the change in itself, but there is no guidance about whether it should happen. It's a code rewrite with no semantic effect. It's also a reasonable thing to do, but there are surely many places throughout std that could have a similar change applied. That in turn could lead to many tiny CLs coming in, creating an undue load on maintainers and the trybots to check them.

It's reasonable to suggest that new significant packages, such as "slices", be used in std to show how best to use them. The standard library acts as an exemplar of good Go style.

So I propose two things:

  1. A policy concisely (if imperfectly) defining when a new package, function, or language feature be deployed throughout std.
  2. A decision that when such a possibility arises, it be done wholesale, in one or maybe a handful of big CLs, to get it done quickly and efficiently.
@gopherbot gopherbot added this to the Proposal milestone May 18, 2024
@earthboundkid
Copy link
Contributor

earthboundkid commented May 20, 2024

An example of this not happening is cmp.Or, which I added, but I didn't go back and look for all the places where it could be swapped in. (It also wasn't clear if doing that would make sense stylistically, since it's usually slower than a simple if statement anyway.) A lot of times someone will propose something and say "Look we do this N times in the standard library with unexported internal functions" and that's what gives it the evidence it needs to be approved, but actually fixing the N unexported functions is a secondary task that ends up out of scope for the proposer, who just wants to add their thing.

With reflect.TypeFor (which Josh proposed and I implemented), the refactor did actually happen, but it was separate from my initial implementation.

@seankhliao
Copy link
Member

consider also #32816
perhaps significant new additions should have corresponding updates in go fix?

@aimuz
Copy link
Contributor

aimuz commented May 21, 2024

There is also a new feature, iter, which can be rewritten in most places as for range b.N

@bjorndm
Copy link

bjorndm commented May 21, 2024

As long as the changes are benchmarked to be of same performance and memory useas before, this seems like a good idea.

@aimuz
Copy link
Contributor

aimuz commented May 21, 2024

In most cases, there is a performance gain of about 2% and fewer allocation

@robpike
Copy link
Contributor Author

robpike commented May 22, 2024

Please confine your comments to discussion of the proposal itself rather than turning this thread into a list of recent changes, which can be curated just from scanning git log.

@ianlancetaylor
Copy link
Contributor

I agree that point 1 is important. Point 2 seems less important to me. As an experiment inspired by this proposal I tried writing a large CL to convert from sort.Sort to slices.SortFunc where appropriate. The result was https://go.dev/cl/587655, which was somewhat awkward to write and definitely hard to review. Nobody wants to look at a change to 89 files where some of the changes are trivial and some require thought. A purely mechanical change can be large, but I think it's reasonable to break up a more complex one into a series of small targeted CLs.

So to me the important point is a way to decide the direction that we want to go, and a way to avoid backsliding and, especially, changing back and forth. I tentatively suggest a Transitions wiki page that describes the intended direction of the source tree. For example, the GCC project has https://gcc.gnu.org/wiki/Partial_Transitions.

@robpike
Copy link
Contributor Author

robpike commented May 23, 2024

Thanks for doing the experiment @ianlancetaylor. You don't discuss the breakdown but let's say it's 50:50. I'd happily review a CL containing consistent trivial fixes to 45 files, or even 88, followed by however many it takes to get the trickier ones in, which could presumably still be grouped a bit.

The key point is to get it done by an approved process rather than saying, "let's do this" and then not seeing it through efficiently.

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

No branches or pull requests

7 participants