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: clarify Type.IsPtr and Isptr #15028

Closed
josharian opened this issue Mar 30, 2016 · 7 comments
Closed

cmd/compile: clarify Type.IsPtr and Isptr #15028

josharian opened this issue Mar 30, 2016 · 7 comments

Comments

@josharian
Copy link
Contributor

func (t *Type) IsPtr() bool was introduced for the SSA backend. It reports (roughly) whether type is pointer-shaped, so it returns true for e.g. maps and channels. The "roughly" qualifier is because there are some things that are pointer-shaped (struct { i *int }) for which it returns false.

var Isptr [NTYPE]bool is used extensively in the frontend. It is set to true for only TPTR32 and TPTR64.

This doesn't matter directly to my type encapsulation efforts, but it seems confusing/dangerous. Here's a proposal. I'd like feedback before I implement it, because the CL will be large, albeit mostly automated:

  • Rename the IsPtr method to IsPtrShaped and adjust ssa.Type appropriately. Use docs to explain the caveat noted above.
  • Eliminate the Isptr var in favor of an IsPtr method on Type.
  • Add an IsUnsafePtr method on Type to highlight that IsPtr does not include TUNSAFEPTR.
  • Bonus: Add checks to IsPtr to ensure that the right pointer width is in use everywhere.

@randall77 @mdempsky @bradfitz

@josharian josharian self-assigned this Mar 30, 2016
@josharian josharian added this to the Unplanned milestone Mar 30, 2016
@griesemer
Copy link
Contributor

Seems like a good idea to me, regarding the front-end. I let @randall77 comment on the SSA change.

Sometimes IsPtr may need to include IsUnsafePtr, and sometimes not. Depending on how many use cases of each kind you may want to make IsPtr include unsafe.Pointer, or not. It'll be obvious enough once the rewrite is done.

@mdempsky
Copy link
Contributor

@mdempsky
Copy link
Contributor

Aside: I'm wondering if IsPtrShaped should include the extra checks for single-element structs and arrays that isdirectiface has. It's not obvious to me that there are use cases for wanting to identify channel, map, and func types because they fit into a single pointer value, but not single element struct/arrays that do too. I haven't looked much into that yet though.

@josharian
Copy link
Contributor Author

FYI: https://go-review.googlesource.com/#/c/21232/

I guess I'm in good company. :) The methods could use some docs, a la the discussion above. Are you also planning to send a CL switching the front-end to use IsPtr?

Aside: I'm wondering if IsPtrShaped should include the extra checks for single-element structs and arrays that isdirectiface has.

It's complicated. See the comment at case OEFACE in ssa.go's expr method, around line 1994 right now.

gopherbot pushed a commit that referenced this issue Mar 30, 2016
Previously, t.IsPtr() reported whether t was represented with a
pointer, but some of its callers expected it to report whether t is an
actual Go pointer. Resolve this by renaming t.IsPtr to t.IsPtrShaped
and adding a new t.IsPtr method to report Go pointer types.

Updated a couple callers in gc/ssa.go to use IsPtr instead of
IsPtrShaped.

Passes toolstash -cmp.

Updates #15028.

Change-Id: I0a8154b5822ad8a6ad296419126ad01a3d2a5dc5
Reviewed-on: https://go-review.googlesource.com/21232
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@mdempsky
Copy link
Contributor

Are you also planning to send a CL switching the front-end to use IsPtr?

Yeah. I was going to try to get rid of all the Isslice, Isfixedarray, Isptr stuff in favor of the Type methods that were added for SSA.

@josharian
Copy link
Contributor Author

Fabulous. We are in danger of stepping on each others' toes with large scale automated rewrites, but I think/hope that mine should be done pretty soon.

Type is actually getting closer to being extractable into its own package. Hard to believe.

@josharian
Copy link
Contributor Author

I think the only todo item from this issue is "Bonus: Add checks to IsPtr to ensure that the right pointer width is in use everywhere." and that's really a part of #15032, so closing. @mdempsky et al feel free to
re-open if I missed something.

@golang golang locked and limited conversation to collaborators Apr 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants