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

Jump to bottom

cmd/compile: use Node.Right rather than Node.RList for OAS2* nodes (cleanup) #32293

Closed
griesemer opened this issue May 28, 2019 · 6 comments
Closed

cmd/compile: use Node.Right rather than Node.RList for OAS2* nodes (cleanup) #32293

griesemer opened this issue May 28, 2019 · 6 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Unplanned

Comments

@griesemer
Copy link
Contributor

griesemer commented May 28, 2019 •
edited

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.
@griesemer griesemer added the Suggested Issues that may be good for new contributors looking for work to do. label May 28, 2019
@griesemer griesemer added this to the Unplanned milestone May 28, 2019
@mdempsky
Copy link
Member

mdempsky commented May 28, 2019 •
edited

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:

  1. In typecheck.go, finding the places where Op==OAS2 is changed to OAS2FUNC/OAS2RECV/OAS2MAPR/OAS2DOTTYPE (looks like typecheckas2), and moving n.Rlist.First() to n.Right at the same time.
  2. Finding all the rest of the compiler code that checks for those OAS2* nodes, and replacing n.Rlist.First() or n.Rlist.SetFirst(x) with n.Right and n.Right = x, respectively.
  3. Update the documentation in syntax.go to reflect the new Node convention.

Make sure to update esc.go too and test that it works with -gcflags=all=-newescape=false, assuming it hasn't been removed yet.

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 28, 2019
@Zyqsempai
Copy link

I will take care of this one!

@Zyqsempai
Copy link

@mdempsky I did like you described above, "replaced all n.Rlist.First() or n.Rlist.SetFirst(x) with n.Right and n.Right = x, respectively." in all the compiler folder.
And now I get internal compiler error: unexpected call op: CONVNOP on ./all.bash
Does it say something to you?
Not sure if i should open a PR with not working code, but maybe it will help you to identify what i missed.?

@griesemer
Copy link
Contributor Author

@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.

@Zyqsempai
Copy link

@griesemer Thanks a lot. Will check everything again.

@gopherbot
Copy link

Change https://golang.org/cl/197817 mentions this issue: cmd/compile: use Node.Right for OAS2* nodes (cleanup)

@golang golang locked and limited conversation to collaborators Sep 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@mdempsky @julieqiu @Zyqsempai @griesemer @gopherbot