-
Notifications
You must be signed in to change notification settings - Fork 570
[go1.20] Removing generic overrides that are no longer needed #1294
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
I think we may have to hold off on that one until generics are no longer guarded by the GOPHERJS_EXPERIMENT flag. Otherwise the standard library would no longer build for those who don't have the experiment enabled. |
@nevkontakte In the go1.20 branch, the standard libraries don't built without the experiment flag as is. I have a bunch of overrides for a generic-less go1.20 in my WIP for g1.20. If you want to, I could pull those out and get them a PR for the go1.20 branch. I was getting close to a generic-less go1.20 (with the exception of that So, would you like me to cut some PR's for an almost generic-less go1.20, I could do that? Or I could just hold off on removing these for now but also not add more generic-less overrides, whatever helps the most. |
We'll hold off on these changes until generics and go1.20 is further along. |
The rule of thumb is that master branch must be usable by the general population at all times, even between releases. As such, we have two options:
I don't think I have a strong preference one way or another. It really depends on how deeply embedded generics are in the 1.20 version, and you probably have a better grasp on this than me :) |
I was thinking we'd hold off on 1.20 until generics are done. The big thing is the
I'm not fond of any of those, so we'll just have to hold off on 1.20 until generics are done and make go1.20 branch a "needs generics enabled" branch until then. |
Alright, I have no objections in this case. I'll take another look at this PR when I have a bit more time, and see if we can merge it. |
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.
Could you please double-check that this PR doesn't actually introduce any regressions? I am pretty sure atomic.Pointer
overrides are still necessary, and I suspect that at least some of the overrides in the nistec
package need to stay.
files []*File | ||
|
||
// replaced atomic.Pointer[File] for go1.19 without generics. | ||
last atomicFilePointer |
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.
Removing this override may be premature, since in their current state generics don't support instantiations across package boundaries.
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.
It didn't seem to, but I'm still working through the errors for go1.20 in my WIP PR. There seems to be a few that may be related to removing the crypto overrides (nistec
, like you said above) and at least one that looks like it may be related to atomic.Pointer. (I haven't gotten very far into figuring those out yet so I'm not 100% sure if they are related or not)
I was just so excited that generics worked for so many issues that I probably am just being preemptive on this. Like we discussed before, we can hold off on this. We'll still try for a go1.20 with generics, but until then I don't want to add problems while generics is being worked on.
I'll put this back into Draft and later when you think it is a better time, we can the try it again 😄
We'll try to remove the overrides later in smaller chunks once the generics are further along. go1.20 branch will still need |
We'll hold off removing the overrides until generics is further along
and we can assess the impact of removing individual overrides
With the amazing updates made to master for generics, many of the generics overrides for go1.19 and go1.20 are no longer needed. There are a few that I have not removed because they hit some strange bugs that we can look into in another ticket.
We're getting closer, it was really cool to see
go1.20.14 install -v && gopherjs build -v net/http
pass without issue.gopherjs test -v fmt log
still fails but that's because of other go1.20 changes that's I'm working through.This is part of #1270