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

Open
martisch opened this Issue Aug 25, 2018 · 1 comment

Comments

Projects
None yet
3 participants
@martisch
Member

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

@griesemer

This comment has been minimized.

Contributor

griesemer commented Sep 5, 2018

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

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