-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: use Node.Right rather than Node.RList for OAS2* nodes (cleanup) #32293
Comments
Marking as "help wanted" since this is probably a reasonable task for folks modestly familiar with the Go compiler and wanting to contribute. It should mostly involve:
Make sure to update esc.go too and test that it works with |
I will take care of this one! |
@mdempsky I did like you described above, |
@Zyqsempai Thanks for volunteering to make this change. Because this is a very low-priority issue (I filed this mostly as a reminder for myself for a future cleanup), please keep in mind that it may not make a lot of sense for a Go Team member to spend time tracking down a bug you have introduced in your change - there's simply too much higher-priority work to be done. Please do not send a non-working PR, we're not going to fix it for you. Hopefully this makes sense. That said, in this specific case, the tricky part is to make this change only for nodes with the specified Op values. If the change is made for any other node, arbitrary things might happen. Perhaps you overlooked a case somewhere. I'd go back and manually double-check every single change and corresponding Node Op value. If you're still stuck, perhaps a community member can help out. It may also be that we overlooked something and this change is more subtle than it looks like. Thanks. |
@griesemer Thanks a lot. Will check everything again. |
Change https://golang.org/cl/197817 mentions this issue: |
Cleanup suggestion (per e-mail discussion between @griesemer and @mdempsky):
The nodes with ops OAS2FUNC, OAS2RECV, OAS2MAPR, OAS2DOTTYPE, and OASOP, use an RList, but (at least) in the documentation the RHS looks like a single node.
They could be (likely) switched to using the Right field, making that field access slightly more efficient.
The text was updated successfully, but these errors were encountered: