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: refactor access to Val values to use node methods #27212

Closed
martisch opened this issue Aug 25, 2018 · 3 comments
Closed

cmd/compile: refactor access to Val values to use node methods #27212

martisch opened this issue Aug 25, 2018 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@martisch
Copy link
Contributor

martisch commented Aug 25, 2018

For accessing e.g the string value of a node n that is a string literal the cmd/compile code base (walk.go, typecheck.go...) often uses n.Val().U.(string). This exposes a lot of details about how constants and literals are implemented in the compiler to code that is mostly operating on nodes.

Some types such as CTBOOL and CTINT already have node methods ((n *Node) Bool() and (n *Node) Int64()) to extract the value the node represents.

We can abstract away how values are represented with node methods and use these outside const.go instead of relying on the implementation details of how values are stored in nodes.
All implementation details of how values are operated on inside nodes can be confined to const.go and the specific files for helper structures such as MpInt and MpFlt.

This should make the compiler code more readable, maintainable and help efforts to easier replace how the compiler stores and handles constants e.g. #4617.

Alternative

Instead of methods on the level of node they could be methods on the level of Val.
So instead of n.StringVal() for node n. It could be n.Val().StringValue().
This would keep the set of methods for operating on the subset of nodes with values separated instead of merging them into the pool of general node methods.
The downsides for the alternative would be:

  • more verbose
  • extra function call if we later decide to make a new constant implementation just a direct interface field of a node with no overlap with other node info.
  • likely larger binary size due to more Val() function calls all around the code base

cc @josharian @griesemer

@martisch martisch added this to the Unplanned milestone Aug 25, 2018
@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 30, 2018
@griesemer
Copy link
Contributor

@martisch I think you should experiment with this and send a CL if successful. I am going to assign this to you.

@martisch
Copy link
Contributor Author

martisch commented Dec 3, 2020

/cc @mdempsky

I think with the CLs to make the compiler use go/constant package this will be resolved.

@martisch martisch removed their assignment Dec 3, 2020
@mdempsky
Copy link
Member

mdempsky commented Dec 3, 2020

Yeah, I think this is basically done now on the dev.regabi branch. We can probably continue improving the API, but values are now opaquely represented using constant.Value.

@mdempsky mdempsky closed this as completed Dec 3, 2020
@golang golang locked and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants