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: remove Type.Vargen #19878

Open
josharian opened this Issue Apr 7, 2017 · 2 comments

Comments

Projects
None yet
3 participants
@josharian
Contributor

josharian commented Apr 7, 2017

The only place that Type.Vargen is written is in typechecking, where is it set with t.Vargen = n.Name.Vargen. A few lines later, we also provide t with a reference to n: t.Nod = asTypesNode(n). So Type.Vargen could be implemented as a lazy lookup of t.Nod.Name.Vargen, defaulting to zero if t.Nod or t.Nod.Name are nil.

I'd send a CL, but amusingly, yesterday's refactoring made it more difficult, because Nod fields are now inaccessible from Types. If we switch to using (say) an interface for Type.Nod, then the Node Vargen lookup could become part of that interface. Conveniently, removing Type.Vargen would almost pay for that, size-wise.

cc @mdempsky @griesemer

@griesemer

This comment has been minimized.

Contributor

griesemer commented Apr 7, 2017

Apologies for making things more difficult. Hopefully this is temporary. To a large extent, types, symbols, and scopes (dclstack etc) should be able to stand alone, which hopefully should allow significant disentanglements in the long run. In the short run, things are a bit ugly.

@josharian

This comment has been minimized.

Contributor

josharian commented Apr 7, 2017

Apologies for making things more difficult.

No worries. All in the name of progress. :) I'm sure my incessant ongoing refactoring of the last weeks has caused no shortage of rebase pain to others.

@andybons andybons added the NeedsFix label Apr 11, 2018

@andybons andybons added this to the Unplanned milestone Apr 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment