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

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

Open
griesemer opened this issue May 28, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@griesemer
Copy link
Contributor

commented May 28, 2019

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 label May 28, 2019

@griesemer griesemer added this to the Unplanned milestone May 28, 2019

@mdempsky

This comment has been minimized.

Copy link
Member

commented May 28, 2019

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.

@Zyqsempai

This comment has been minimized.

Copy link

commented May 29, 2019

I will take care of this one!

@Zyqsempai

This comment has been minimized.

Copy link

commented May 30, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@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

This comment has been minimized.

Copy link

commented May 30, 2019

@griesemer Thanks a lot. Will check everything again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.