-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
Comments
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. |
consider also #32816 |
There is also a new feature, iter, which can be rewritten in most places as for range b.N |
As long as the changes are benchmarked to be of same performance and memory useas before, this seems like a good idea. |
In most cases, there is a performance gain of about 2% and fewer allocation |
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. |
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 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. |
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. |
Related: #70815. |
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:
The text was updated successfully, but these errors were encountered: