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: clean more up after ssa.Type -> types.Type refactoring #20304

Closed
josharian opened this issue May 9, 2017 · 21 comments

Comments

Projects
None yet
5 participants
@josharian
Copy link
Contributor

commented May 9, 2017

CL 42145 switched package ssa to use types.Type for types. Further cleanup is now possible: Eliminating (in part or in full) ssa.Types, streamlining/eliminating some types.Type methods, moving universe initialization to package types. This is a reminder to do that.

cc @randall77 @mdempsky @griesemer

@josharian josharian added this to the Go1.10 milestone May 9, 2017

@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 29, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

Adding HelpWanted since this is some low hanging fruit that folks interested in stepping into the compiler could easily help with.

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2018

I’d like to have a try at this - your “low hanging fruit” remark gives me some confidence.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2018

Go for it!

Bear in mind that the tree is closed until announced on golang-dev, probably Feb 1. Since these should involve no functional changes, you might find toolstash-check to be helpful. Err on the side of smaller changes rather than one big change; it eases reviewing. If you have any questions, feel free to ask here.

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

Happy to look. (Expect possible delays from me; my schedule is quite fragmented.)

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

Did you mean to attach or link to a document? I don't see it.

It's probably not worth doing a bunch of mass renames, unless it will bring a significant readability improvement. We have done significant renames before, and will surely do some again, when they matter. We've never formalized how it works, but if you think we should do some renames, I'd suggest something like:

  • Make a list of renames you want to do.
  • Convert that into a script that calls gorename a bunch. Test it out, both that the renames apply cleanly and that the results look good.
  • Post that list somewhere, like an issue or golang-dev, for review, since it is easier to review such a list than the resulting CLs.
  • Consider asking someone from the core team to apply the renames. (It is a pain to review large automated refactoring changes. This is one of the rare cases in which I treat review requests significantly differently depending on how well I know the author. Known authors can be rubber-stamped, unknown authors require painful manual reviews.)
  • Do the renames during a quiet period in the tree. The ideal time is basically right now--right before the tree opens for general changes. Then it is likely to apply cleanly and not disrupt too much other work in progress. There's usually a golang-dev email that goes out before the tree opens to coordinate work such as this.

That's probably more than you wanted to know, but hope it is helpful. :)

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

Hmm, I can’t see it. How are you enclosing it? I think github strips email attachments.

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2018

Sharing a google doc is fine. Or paste it into a gist (gist.github.com)? Or probably best, just paste all the contents here.

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2018

Eliminating ssa.Types would cause a large volume of renaming Bool to TBOOL, Int8 to TINT8 etc and I don’t really see much value in that, at least in the short term

What about just adding more "convenience constants" in the right place? See cmd/compile/internal/gc/types.go.

Moving universe.go to types package
This can be done easily enough but I don’t see that it will have any benefit.

At a minimum it would help with the package ssa tests, which currently manually initialize some bits of the universe. And it'd be better in the sense that that is where it belongs, and having things where they belong tends to pay dividends.

Leave a “small” align.go with just func Rnd in gc?

Or just move Rnd to some other grab-bag util file, like cmd/internal/obj/util.go. It could also really use a doc string. Also, is the cmd/link/internal/ld Rnd identical or not? If identical, it could use the cmd/internal/obj Rnd.

Used only to define method *Type.Symbol in types/type.go.
This method is apparently not used.

Used in writebarrier.go, I believe.

The rest of the constants - FErr, FmtLeft, FmtUnsigned, FmtFlag, fmtMode and functions - sconv, tconv, symFormat, typeFormat all come from gc/fmt.go.
They could be “moved” to the types package (utils.go?) and “copied” to gc/types.go as “convenience constants (and functions)”. Not an ideal solution but workable.

fmt.go is a train wreck. I would touch/modify it as little as possible. convenience constants seem fine, though.

As for Widthptr and Ctxt, could part of gc/main.go be moved back up to cmd/compile/main.go so that “architecture” (and maybe other) values can be propagated to both types and gc? Either that or have a types module up at that level. It seems that the types package needs some initialisation.

That seems reasonable to me, but I'd like @mdempsky's opinion.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2018

And if @griesemer or @mdempsky and I disagree, go with what they say. :)

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

A careful assessment is helpful, even if it doesn’t result in code changes. Sometimes more so. So thank you for that.

If you think there are any clear, worthwhile wins, please go ahead and send changes and then we’ll call this fixed enough.

And then maybe take your interest and enthusiasm to another issue? :) Those labeled suggested and helpwanted are good places to trawl.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 15, 2018

Change https://golang.org/cl/94256 mentions this issue: gc: move the SSA local type definitions to a single location.

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

Disappointingly, I was only able to come up with a minor change which confines the definition of the SSA type pointer struct to a single location - ssa/config.go - rather than two places. This is a very minor code improvement.
My final take on the main aims of this issue are:

  1. Renaming the ssa type definitions would cause a very large number of edits - I don't think that renaming would lead to any great improvement in readability so I understand that it's probably not worth doing (now). If I'm wrong about this, I'd be happy to have a go at it.
  2. gc/universe.go has quite a large number of dependencies on code scattered through gc so a straight-forward move of universe from gc to types would make the "ugly initialisations" in gc/main and ssa/export_test much worse than they are now. So a more extensive refactoring seems to be needed.
@griesemer

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

@ChrisALiles At least for your point 2) I arrived at the same conclusion which is why I didn't move universe to internal/types in the first place. Re: 1), I am not surprised. Making incremental changes in the front-end have become increasingly difficult due to existing interdependencies.

I think a long-term plan requires more significant rewrites.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2018

@ChrisALiles thanks for seeing it through. I hope you continue to dig around elsewhere as well.

@gopherbot gopherbot closed this in 4f5389c Feb 27, 2018

@golang golang locked and limited conversation to collaborators Feb 27, 2019

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