Issue 7071058: code review 7071058: exp/ssa: API and documentation. (Closed)
Descriptionexp/ssa: API and documentation.
Patch Set 1 #Patch Set 2 : diff -r 1399878c6731 https://code.google.com/p/go/ #
Total comments: 28
Patch Set 3 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #Patch Set 4 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #Patch Set 5 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #Patch Set 6 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #
Total comments: 79
Patch Set 7 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #
Total comments: 39
Patch Set 8 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #Patch Set 9 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #Patch Set 10 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #Patch Set 11 : diff -r 9a3e56fe880c https://code.google.com/p/go/ #
Total comments: 26
Patch Set 12 : diff -r 76dc0d7cec07 https://code.google.com/p/go/ #Patch Set 13 : diff -r fd51d6123dfa https://code.google.com/p/go/ #MessagesTotal messages: 20
some superficial comments - need to spend more time on this later (not today) - gri https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:14: type ( Organizational note: The type decls and comments are so large that I would do a new type decl for each one instead of having everything indented all the way. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:28: // more detail than files[*].Name.Name (a.k.a. p.Name). This last part is odd - leave out. Package names are know to be not unique. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:32: AddPackage(importPath string, files map[string]*ast.File) (p *Package, err error) fyi: I am considering moving away from map[string]*ast.File to []*ast.File. The problem with the map is that it doesn't produce the same iteration order each time which is problematic for testing. As a consequence, now I always have to sort. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:56: // for the Universals scope. Universals? It's the Universe scope. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:167: Insn interface { Please find a different shortcut - Isns means everything and nothing. Instr seems pretty clear. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:224: Signature *types.Signature s/Signature/Type/ for the field name? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:226: Position token.Position // location of the definition s/Position/Pos/ for the field name? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:228: Package *Package // enclosing package; nil for synthetic methods s/Package/Pkg/ for the field name? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:235: recvfield *ast.FieldList s/recvfield/recvField/ or just recv? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:236: paramfields *ast.FieldList ditto, and below https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:259: Preds, Succs []*BasicBlock // predecessors and successors Do you need both directions? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:279: typ types.Type types.Pointer ? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:298: typ types.Type types.Pointer ? https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:420: // TODO(adonovan): consider dropping Edges[i].Block since ah!
Sign in to reply to this message.
Great comments---thanks. All 'done' as suggested except where noted. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:14: type ( On 2013/01/09 22:55:01, gri wrote: > Organizational note: The type decls and comments are so large that I would do a > new type decl for each one instead of having everything indented all the way. Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:28: // more detail than files[*].Name.Name (a.k.a. p.Name). On 2013/01/09 22:55:01, gri wrote: > This last part is odd - leave out. Package names are know to be not unique. Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:32: AddPackage(importPath string, files map[string]*ast.File) (p *Package, err error) On 2013/01/09 22:55:01, gri wrote: > fyi: I am considering moving away from map[string]*ast.File to []*ast.File. The > problem with the map is that it doesn't produce the same iteration order each > time which is problematic for testing. As a consequence, now I always have to > sort. Fine by me. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:56: // for the Universals scope. On 2013/01/09 22:55:01, gri wrote: > Universals? It's the Universe scope. Oops, a hangover from the terminology I used in GCLx. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:167: Insn interface { On 2013/01/09 22:55:01, gri wrote: > Please find a different shortcut - Isns means everything and nothing. > > Instr seems pretty clear. Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:224: Signature *types.Signature On 2013/01/09 22:55:01, gri wrote: > s/Signature/Type/ for the field name? This would conflict with the Type() method of the Value interface, which *Function implements. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:226: Position token.Position // location of the definition On 2013/01/09 22:55:01, gri wrote: > s/Position/Pos/ for the field name? I wanted to avoid the suggestion that this was a token.Pos, which is a different datatype, confusingly. In any case, all the source position stuff is likely to change. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:228: Package *Package // enclosing package; nil for synthetic methods On 2013/01/09 22:55:01, gri wrote: > s/Package/Pkg/ for the field name? Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:235: recvfield *ast.FieldList On 2013/01/09 22:55:01, gri wrote: > s/recvfield/recvField/ or just recv? Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:236: paramfields *ast.FieldList On 2013/01/09 22:55:01, gri wrote: > ditto, and below Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:259: Preds, Succs []*BasicBlock // predecessors and successors On 2013/01/09 22:55:01, gri wrote: > Do you need both directions? Yes. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:279: typ types.Type On 2013/01/09 22:55:01, gri wrote: > types.Pointer ? Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:298: typ types.Type On 2013/01/09 22:55:01, gri wrote: > types.Pointer ? Done. https://codereview.appspot.com/7071058/diff/2001/src/pkg/exp/ssa/ssa.go#newco... src/pkg/exp/ssa/ssa.go:420: // TODO(adonovan): consider dropping Edges[i].Block since On 2013/01/09 22:55:01, gri wrote: > ah! Does this answer your question about succs/preds above? Or are you expressing an opinion about the TODO? I'm confused.
Sign in to reply to this message.
Hello gri@google.com, iant@google.com, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
Minor comments. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:1: // exp/ssa: SSA representation of Go programs // Package ssa ... https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:36: type Builder interface { I don't understand why this is an interface and not a struct. You may also want to consider a name like Program. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:89: // Package represents a single analyzed package, containing Members s/represents/is/ https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:105: // Member represents a member of a package, implemented by *Literal, s/represents/is/ https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:114: // An Id identifies the name of a field of a struct type, or the name ID is more common. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:132: // A method set contains all the methods whose receiver is either T or // A MethodSet https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:213: type Instr interface { Instruction? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:270: Signature *types.Signature In the types below you use Type_, is a Signature significantly different? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:387: // A named Value holding the address of a var defined inside a package // A Global is a named Value https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:787: // Helper for Select. // SelectState ... https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1034: // Register is a mix-in embedded by all SSA values that are also Odd to see these described as mix-ins. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1046: Num int // "name" of virtual register, e.g. "t0". Not guaranteed unique. If Num cannot safely be used to identify registers, why expose it? Isn't Name good enough for debugging? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1130: // Returns the package-level function of the specified name, or nil // Func returns https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1137: // Returns the package-level variable of the specified name, or nil // Var returns
Sign in to reply to this message.
Thanks, all good comments. All done, except the Builder/builder merge; will wait for gri's thoughts. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:36: type Builder interface { On 2013/01/17 16:08:49, crawshaw1 wrote: > I don't understand why this is an interface and not a struct. You may also want > to consider a name like Program. My intention was not to expose the concrete builder type and to gather its interface together in once place; but perhaps this isn't idiomatic Go. I'll wait to see what gri says before I start editing but I'm happy to make your change. As to the name: the point is that the Builder can (and should) be thrown away once the Packages are created. I think I may yet want something like Program which is a detachable container of the "proof but not the lemmas", so to speak: the Packages and MethodSets. The Builder API is certain to change as we learn more. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:89: // Package represents a single analyzed package, containing Members On 2013/01/17 16:08:49, crawshaw1 wrote: > s/represents/is/ Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:105: // Member represents a member of a package, implemented by *Literal, On 2013/01/17 16:08:49, crawshaw1 wrote: > s/represents/is/ Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:114: // An Id identifies the name of a field of a struct type, or the name On 2013/01/17 16:08:49, crawshaw1 wrote: > ID is more common. Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:132: // A method set contains all the methods whose receiver is either T or On 2013/01/17 16:08:49, crawshaw1 wrote: > // A MethodSet Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:213: type Instr interface { On 2013/01/17 16:08:49, crawshaw1 wrote: > Instruction? Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:270: Signature *types.Signature On 2013/01/17 16:08:49, crawshaw1 wrote: > In the types below you use Type_, is a Signature significantly different? A Signature is a kind of Type: the type of a function. I could have called the field Type_, but I was happy with this. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:387: // A named Value holding the address of a var defined inside a package On 2013/01/17 16:08:49, crawshaw1 wrote: > // A Global is a named Value Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:787: // Helper for Select. On 2013/01/17 16:08:49, crawshaw1 wrote: > // SelectState ... Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:787: // Helper for Select. On 2013/01/17 16:08:49, crawshaw1 wrote: > // SelectState ... Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1034: // Register is a mix-in embedded by all SSA values that are also On 2013/01/17 16:08:49, crawshaw1 wrote: > Odd to see these described as mix-ins. Isn't that exactly what they are? I don't know of a canonical definition, but the Wikipedia definition seems as good as any, and couldn't be more applicable to this example: "In object-oriented programming languages, a mixin is a class that provides a certain functionality to be inherited or just reused by a subclass, while not meant for instantiation (the generation of objects of that class). Inheriting from a mixin is not a form of specialization but is rather a means of collecting functionality. A class or object may "inherit" most or all of its functionality from one or more mixins, therefore mixins can be thought of as a mechanism of multiple inheritance." https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1046: Num int // "name" of virtual register, e.g. "t0". Not guaranteed unique. On 2013/01/17 16:08:49, crawshaw1 wrote: > If Num cannot safely be used to identify registers, why expose it? Isn't Name > good enough for debugging? In publicising Register I was trying to avoid hiding fields that make it challenging for clients to construct instances of Instruction. But I think in this case you are right. Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1130: // Returns the package-level function of the specified name, or nil On 2013/01/17 16:08:49, crawshaw1 wrote: > // Func returns Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1137: // Returns the package-level variable of the specified name, or nil On 2013/01/17 16:08:49, crawshaw1 wrote: > // Var returns Done.
Sign in to reply to this message.
Hello gri@google.com, iant@google.com, crawshaw@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:1: // exp/ssa: SSA representation of Go programs drop this line https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:5: // The package exp/ssa defines a representation of the elements of Go // Package ssa defines ... And make that the first line of the comment, so it's the summary. Move the ALL CAPS WARNING after the summary. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:3: // This package defines a high-level intermediate representation for you already have doc.go. drop this comment.
Sign in to reply to this message.
https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:1: // exp/ssa: SSA representation of Go programs On 2013/01/17 19:12:50, bradfitz wrote: > drop this line Done. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:5: // The package exp/ssa defines a representation of the elements of Go On 2013/01/17 19:12:50, bradfitz wrote: > // Package ssa defines ... > > And make that the first line of the comment, so it's the summary. > > Move the ALL CAPS WARNING after the summary. Done. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:3: // This package defines a high-level intermediate representation for On 2013/01/17 19:12:50, bradfitz wrote: > you already have doc.go. drop this comment. Done.
Sign in to reply to this message.
First round. More to come. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:36: type Builder interface { I'd agree with crawshaw1. Is there only one Builder or is the Builder creating new ones recursively? If the latter, using an interface makes sense if one wanted to use a different builder. But is that actually feasible? Or even desirable? If not, just export the concrete type but keep the methods. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:39: // Values for the package. Returns a new SSA Package s/Returns a/The result is a/ https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:49: // TODO(adonovan): not idempotent; perhaps a bad sign. what's the reason? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:56: // functions and init-blocks in package p and its init blocks are functions - leave away https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:94: Types *types.Package // the type checker's package object for this package. I don't see a good reason for this field to be called Types. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:95: ImportPath string // e.g. "sync/atomic" Shouldn't this be the same as Types.Path? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:96: Position token.Position // position of an arbitrary file in the package [this will change] Why is this exported? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:99: Init *Function // the package's init function A package can have multiple init() functions. Shouldn't this be []*Function? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:114: // An Id identifies the name of a field of a struct type, or the name On 2013/01/17 17:26:28, adonovan wrote: > On 2013/01/17 16:08:49, crawshaw1 wrote: > > ID is more common. > > Done. I don't know that this is true. Personally, I'd chose Id. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:129: // instances returned by the type-checker do not have this property.) QualifiedNames must be compared with IsSame - which does have this property. But I agree that a value cannot be used directly. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:147: // interface types. It contains methods for the pointer receiver The last sentence sounds odd. I thought that this is already clear from MethodSet. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:158: // The following types are assignable to Value: Capture, Parameter, This enumeration is likely to get out of sync over time because the comment is not updated. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:205: // The following types are assignable to Instr: If, Jump, Ret, Alloc This enumeration is likely to get out of sync over time because the comment is not updated. I would leave it away. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:244: // assignable. Should explain what a function assignment looks like. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:275: Params []*Parameter // inputs these line comments are not adding information https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:276: Captures []*Capture // captures s/Captures/Context/ ? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:310: // The Outer field records the correspondence of the value as known to Outer is just the value in the context, isn't it? Seems a bit complicated a description. A line comment next to the field might suffice. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:314: // The referent of a capture is a Parameter, Alloc or another Capture a line comment next to the field may be all that's needed https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:351: // interface, map, channel, pointer, slice, or function. what a bout nil (untyped nil)? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:353: // All source-level constant expressions are represented by a Literal This would be enough comment. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:361: // constants. The dynamic type of Value is the smallest type that can Instead of repeating this, just refer to the types package. If there's a change there, this comment may get out of date. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:390: // Global implements the Value interface. Instead of having this uncheckable comment, why not add an empty ImplementsValue() (or AValue) method? That's declaring the relationship explicitly. Apply everywhere. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:396: // Set transiently during building, then cleared. make it a line comment https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:462: // dropping Edges[i].Block since block.Preds[i] should suffice. agreed, and then Edges can just be []Value https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:488: Register surprisingly litte commentary here compared to elsewhere
Sign in to reply to this message.
Some more comments. I feel that the API is drowned out by documentation. I don't know how much needs to be spelled out in detail. For one, w/o an excellent understanding of Go and its semantics as well as SSA form, this is going to be difficult to use no matter how well documented it is. On the other hand, for somebody familiar with all the details of Go and SSA, not that much documentation may be needed. My fear is that the documentation is inaccurate (perhaps not now, but will be over time) because it's not checked. But overall this looks reasonable to me. Some questions/observations: - does the Type() documentation have to be which each node? (godoc won't do the right thing) - there is duplication is CFG edges (end of a basic block and from last instruction in a basic block) - can it be done smaller? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:21: // primitives in future to facilitate constant-time dispatch of switch s/in/in the/ https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:35: // is not yet implemented. s/is is/is/ https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:51: // Given a Go source package such as this: Instead of this lenghty - unchecked - comment, it would be better to have an example that shows actual code. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:536: // We can't use UnOp because there's no token for it. TODO(adonovan): Why can't you use "x == nil", i.e. a BinOp? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:637: Slots []Value I understand where Slots coming from, but I wonder if there's a better name. Context? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:674: // Type() returns a *types.Slice. this should be with the method declaration https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:709: // Type() returns a *types.Pointer. this should be with the method declaration https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:726: // package-local identifiers and permit compact representations. no need to explain the reasons for the numeric indices in the api https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:756: // Index yields a copy of the element at index Index of array X. just: element at Index of array X. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:853: // value (ok, k, v). If the iterator is not exhausted, ok is true and should ok be last, as would be the case in a comma-ok expression? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:928: Target *BasicBlock This is the same as the successor in that basic block, isn't it? Does it need to be in both places? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:943: True, False *BasicBlock These are the same as the successors in that basic block, isn't it? Do they need to be in both places? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1040: // Embeddable mix-ins used for common parts of other structs. -------------------- they are not really mix-ins https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1042: // Register is a mix-in embedded by all SSA values that are also s/mix-in/struct/ https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1058: // CallCommon is a mix-in embedded by Go, Defer and Call to hold the s/mix-in/struct/ https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1102: // TODO(adonovan): document copying of arguments and receiver. I suggest adding an empty // comment before the type to separate it better from the comment. Your comments are so long, it's hard to see where the declaration starts. Apply everywhere (except perhaps where the comment is short).
Sign in to reply to this message.
Thanks for all the comments; responses inline. PTAL. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:36: type Builder interface { On 2013/01/18 00:07:54, gri wrote: > I'd agree with crawshaw1. Is there only one Builder or is the Builder creating > new ones recursively? If the latter, using an interface makes sense if one > wanted to use a different builder. But is that actually feasible? Or even > desirable? If not, just export the concrete type but keep the methods. Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:39: // Values for the package. Returns a new SSA Package On 2013/01/18 00:07:54, gri wrote: > s/Returns a/The result is a/ Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:49: // TODO(adonovan): not idempotent; perhaps a bad sign. On 2013/01/18 00:07:54, gri wrote: > what's the reason? I've removed this comment; I no longer think it's a significant problem. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:56: // functions and init-blocks in package p and its On 2013/01/18 00:07:54, gri wrote: > init blocks are functions - leave away Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:94: Types *types.Package // the type checker's package object for this package. On 2013/01/18 00:07:54, gri wrote: > I don't see a good reason for this field to be called Types. Any suggestions for a better name? https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:95: ImportPath string // e.g. "sync/atomic" On 2013/01/18 00:07:54, gri wrote: > Shouldn't this be the same as Types.Path? Yes, but see the assertion in builder.go's createPackageImpl. Sometimes it's empty. FWIW this is on the go/types wishlist. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:96: Position token.Position // position of an arbitrary file in the package [this will change] On 2013/01/18 00:07:54, gri wrote: > Why is this exported? Because the test interpreter needs it. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:99: Init *Function // the package's init function On 2013/01/18 00:07:54, gri wrote: > A package can have multiple init() functions. Shouldn't this be []*Function? In the SSA representation the init blocks are concatenated into a single function. I've changed the comment to "... (concatenated) init function". https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:114: // An Id identifies the name of a field of a struct type, or the name On 2013/01/18 00:07:54, gri wrote: > On 2013/01/17 17:26:28, adonovan wrote: > > On 2013/01/17 16:08:49, crawshaw1 wrote: > > > ID is more common. > > > > Done. > > I don't know that this is true. Personally, I'd chose Id. Undone. :) https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:129: // instances returned by the type-checker do not have this property.) On 2013/01/18 00:07:54, gri wrote: > QualifiedNames must be compared with IsSame - which does have this property. But > I agree that a value cannot be used directly. For QualifiedName, IsSame has this property but == does not, so QualifiedNames are not suitable as map keys without additional effort. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:147: // interface types. It contains methods for the pointer receiver On 2013/01/18 00:07:54, gri wrote: > The last sentence sounds odd. I thought that this is already clear from > MethodSet. Indeed; removed. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:158: // The following types are assignable to Value: Capture, Parameter, On 2013/01/18 00:07:54, gri wrote: > This enumeration is likely to get out of sync over time because the comment is > not updated. Yes, but I don't see a satisfactory alternative. Even the explicit declarations of dummy ImplementsValue() functions don't help the reader of the exp/ssa godoc to know which types are Values. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:205: // The following types are assignable to Instr: If, Jump, Ret, Alloc On 2013/01/18 00:07:54, gri wrote: > This enumeration is likely to get out of sync over time because the comment is > not updated. I would leave it away. (Ditto) https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:244: // assignable. On 2013/01/18 00:07:54, gri wrote: > Should explain what a function assignment looks like. Changed to: // Functions are immutable values; they do not have addresses. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:275: Params []*Parameter // inputs On 2013/01/18 00:07:54, gri wrote: > these line comments are not adding information Deleted. I was lured into adding them by gofmt, to keep things neatly aligned. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:276: Captures []*Capture // captures On 2013/01/18 00:07:54, gri wrote: > s/Captures/Context/ ? I've never liked the name "Context"; how about Env, since these are environment bindings for the free variables? (Changed to Env). https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:310: // The Outer field records the correspondence of the value as known to On 2013/01/18 00:07:54, gri wrote: > Outer is just the value in the context, isn't it? Seems a bit complicated a > description. > > A line comment next to the field might suffice. Yes, and done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:314: // The referent of a capture is a Parameter, Alloc or another Capture On 2013/01/18 00:07:54, gri wrote: > a line comment next to the field may be all that's needed I couldn't make it much more concise without making it opaque. In any case I think this may change fairly soon when I implement fully pruned SSA. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:351: // interface, map, channel, pointer, slice, or function. On 2013/01/18 00:07:54, gri wrote: > what a bout nil (untyped nil)? Good point; I've explicitly excluded it. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:353: // All source-level constant expressions are represented by a Literal On 2013/01/18 00:07:54, gri wrote: > This would be enough comment. Not quite sure which part you intended me to delete. Take another look. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:361: // constants. The dynamic type of Value is the smallest type that can On 2013/01/18 00:07:54, gri wrote: > Instead of repeating this, just refer to the types package. If there's a change > there, this comment may get out of date. Done. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:390: // Global implements the Value interface. On 2013/01/18 00:07:54, gri wrote: > Instead of having this uncheckable comment, why not add an empty > ImplementsValue() (or AValue) method? That's declaring the relationship > explicitly. Apply everywhere. Done. I think it's a shame that Go's "duck" typing doesn't provide a better mechanism than this to declare the "implements" relation when you want to do so. This trick of adding a dummy method to the interface has the effect of forcing a change upon all implementations of the interface, thereby mostly nullifying the value of Go's implicit "implements" relation. (In this specific case it's not so bad because the set of Value types is finite; but this probably not even the common case.) https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:396: // Set transiently during building, then cleared. On 2013/01/18 00:07:54, gri wrote: > make it a line comment For symmetry with other occurrences I've left it as-is. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:462: // dropping Edges[i].Block since block.Preds[i] should suffice. On 2013/01/18 00:07:54, gri wrote: > agreed, and then Edges can just be []Value Yes, I think this is probably for the best, but the downside is that the printed form (Name()) of a Phi node must either (a) elide the names of the basic blocks or (b) access them through some kind of context supplied to Name(). I think (b) makes sense since it solves other problems, e.g. printing qualified names of package-level vars only when crossing packages. I've added this to the TODO. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:488: Register On 2013/01/18 00:07:54, gri wrote: > surprisingly litte commentary here compared to elsewhere This class is just the sum of two mix-ins. Not sure what else to say. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go File src/pkg/exp/ssa/doc.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:21: // primitives in future to facilitate constant-time dispatch of switch On 2013/01/18 01:58:14, gri wrote: > s/in/in the/ Good catch. Your English is better than mine. :) https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:35: // is not yet implemented. On 2013/01/18 01:58:14, gri wrote: > s/is is/is/ Good catch--hiding behind the pesky newline. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/doc.go#newc... src/pkg/exp/ssa/doc.go:51: // Given a Go source package such as this: On 2013/01/18 01:58:14, gri wrote: > Instead of this lenghty - unchecked - comment, it would be better to have an > example that shows actual code. Can you be more specific about what "actual code" you have in mind? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:536: // We can't use UnOp because there's no token for it. TODO(adonovan): On 2013/01/18 01:58:14, gri wrote: > Why can't you use "x == nil", i.e. a BinOp? Good call; done. This instruction dated from an earlier time when I misunderstood the nature of untyped nil. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:637: Slots []Value On 2013/01/18 01:58:14, gri wrote: > I understand where Slots coming from, but I wonder if there's a better name. > Context? I dislike the name Context (almost anywhere); how about Bindings, since these are the bindings for each free variable in Fn.FreeVars? https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:674: // Type() returns a *types.Slice. On 2013/01/18 01:58:14, gri wrote: > this should be with the method declaration Agreed, but the method is promoted from Register, so there's nowhere to put it. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:709: // Type() returns a *types.Pointer. On 2013/01/18 01:58:14, gri wrote: > this should be with the method declaration (Ditto) https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:726: // package-local identifiers and permit compact representations. On 2013/01/18 01:58:14, gri wrote: > no need to explain the reasons for the numeric indices in the api Done. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:756: // Index yields a copy of the element at index Index of array X. On 2013/01/18 01:58:14, gri wrote: > just: element at Index of array X. Done. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:853: // value (ok, k, v). If the iterator is not exhausted, ok is true and On 2013/01/18 01:58:14, gri wrote: > should ok be last, as would be the case in a comma-ok expression? I had that originally, but this seemed better since you absolutely have to access ok first--unlike in value,ok expressions. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:928: Target *BasicBlock On 2013/01/18 01:58:14, gri wrote: > This is the same as the successor in that basic block, isn't it? Does it need to > be in both places? (See response to same comment at If.) https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:943: True, False *BasicBlock On 2013/01/18 01:58:14, gri wrote: > These are the same as the successors in that basic block, isn't it? Do they need > to be in both places? Ideally no; but in practice the CFG is built from the If-statement, not the other way around. Furthermore this would also necessitate that If.Name() can access the CFG somehow so it can print intelligible block names. I think it's worth experimenting; I've added a TODO to try this in a follow-up. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1040: // Embeddable mix-ins used for common parts of other structs. -------------------- On 2013/01/18 01:58:14, gri wrote: > they are not really mix-ins Actually, they really are---see my response to crawshaw: the Wikipedia definition of "mix-in" couldn't be more appropriate. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1042: // Register is a mix-in embedded by all SSA values that are also On 2013/01/18 01:58:14, gri wrote: > s/mix-in/struct/ (ditto) https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1058: // CallCommon is a mix-in embedded by Go, Defer and Call to hold the On 2013/01/18 01:58:14, gri wrote: > s/mix-in/struct/ (ditto) https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:1102: // TODO(adonovan): document copying of arguments and receiver. On 2013/01/18 01:58:14, gri wrote: > I suggest adding an empty // comment before the type to separate it better from > the comment. Your comments are so long, it's hard to see where the declaration > starts. Apply everywhere (except perhaps where the comment is short). Done.
Sign in to reply to this message.
I've implemented the changed to Phi/Jump/If we discussed; it's much cleaner: more compact and with fewer opportunities for inconsistency. I had to add the Block and SetBlock methods to Instruction; we were always going to need them sooner or later. Ready for another look. https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/16001/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:462: // dropping Edges[i].Block since block.Preds[i] should suffice. On 2013/01/22 04:37:54, adonovan wrote: > On 2013/01/18 00:07:54, gri wrote: > > agreed, and then Edges can just be []Value > > Yes, I think this is probably for the best, but the downside is that the printed > form (Name()) of a Phi node must either (a) elide the names of the basic blocks > or (b) access them through some kind of context supplied to Name(). I think (b) > makes sense since it solves other problems, e.g. printing qualified names of > package-level vars only when crossing packages. I've added this to the TODO. Done. https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/14006/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:943: True, False *BasicBlock On 2013/01/22 04:37:55, adonovan wrote: > On 2013/01/18 01:58:14, gri wrote: > > These are the same as the successors in that basic block, isn't it? Do they > need > > to be in both places? > > Ideally no; but in practice the CFG is built from the If-statement, not the > other way around. Furthermore this would also necessitate that If.Name() can > access the CFG somehow so it can print intelligible block names. > > I think it's worth experimenting; I've added a TODO to try this in a follow-up. Done.
Sign in to reply to this message.
Updates since last snapshot: - new type Program: this is the toplevel container populated by Builder. - Builder interface replaced by concrete struct. - The If instruction now performs a traditional comparison-based conditional branch, instead of taking a reified boolean operand. - BinOp no longer accepts comparison operators, only arithmetic, bitwise and shifts. - Phi no longer duplicates the predecessor block list of the CFG, which is now accessible as Block().Preds. - token.Pos is now used instead of token.Position: it's more compact. (It requires a token.FileSet to be meaningful; Program has one.)
Sign in to reply to this message.
FYI https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:33: // A Package a single analyzed Go package, containing Members for all s/A package a/A package is a/ https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:60: ImplementsMember() // dummy method to indicate the "implements" relation. Is there any reason to make ImplementsMember an exported method? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:77: // relation == is consistent with identifier equality. (QualifiedName You say that QualifiedName returned by the type checker does not support identifier equality, but then you go ahead and use types.QualifiedName. Something seems out of kilter. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:142: ImplementsValue() Does this method need to be exported? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:179: SetBlock(*BasicBlock) When would it be appropriate for a user of this package to call SetBlock? Should this be an unexported method? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:182: ImplementsInstruction() Exported? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:236: // control (If, Jump or Ret). What about panic? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:261: Type_ *types.Pointer Why do you need Name_ and Type_ fields here? Can't you derive them from Outer? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:273: // the stack frame of the owning function. If the case where It's not clear to me why you discuss "the stack frame of the owning function." In Go terms, calling a function always copies the arguments, just as with assignment. The caller can't refer to the argument, so treating the parameter as a pointer to the caller's argument doesn't seem meaningful. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:286: // (integer, fraction or complex) value. s/fraction/float/ (I think). https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:544: // MakeSlice yields a slice of length Len backed by a newly allocated What are the values of Len and Cap if they are not specified in the program? https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:626: // TODO(adonovan): should we permit X to have type slice? I suppose I would permit X to have type slice. It's not clear why this should not be permitted. After all IndexAddr followed by Load would work for an array as well.
Sign in to reply to this message.
Thanks for the comments. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:33: // A Package a single analyzed Go package, containing Members for all On 2013/01/23 23:41:41, iant wrote: > s/A package a/A package is a/ Done. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:60: ImplementsMember() // dummy method to indicate the "implements" relation. On 2013/01/23 23:41:41, iant wrote: > Is there any reason to make ImplementsMember an exported method? It provides explicit documentation of the implements relation. Another reason to expose the ImplementsFoo method (which doesn't apply in this case since interface Member is a "closed" type) is to permit clients to implement the interface. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:77: // relation == is consistent with identifier equality. (QualifiedName On 2013/01/23 23:41:41, iant wrote: > You say that QualifiedName returned by the type checker does not support > identifier equality, but then you go ahead and use types.QualifiedName. > Something seems out of kilter. Ah, I see the confusion: nowhere in my code do I rely on Id and QN having structural equality; I was just being lazy and reusing the definition. I have copied it out in full now. I've removed all mention of QN. FYI: all instances of QN returned by the type checker have the Pkg field non-nil, which breaks the property described. In constrast all instances of Id have the Pkg field non-nill iff the Name field is unexported. At no point do I in fact convert between them. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:142: ImplementsValue() On 2013/01/23 23:41:41, iant wrote: > Does this method need to be exported? Yes, for documentation. I think it's quite important for the reader to be able to see easily which types are assignable to the principal interfaces. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:179: SetBlock(*BasicBlock) On 2013/01/23 23:41:41, iant wrote: > When would it be appropriate for a user of this package to call SetBlock? > Should this be an unexported method? Anyone doing code transformation will need this method, so it should be exported. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:182: ImplementsInstruction() On 2013/01/23 23:41:41, iant wrote: > Exported? Ditto. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:236: // control (If, Jump or Ret). On 2013/01/23 23:41:41, iant wrote: > What about panic? Currently panic is compiled in a for{} loop to ensure its successor is unreachable. Perhaps panic should have its own Instruction, but we can add that later. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:261: Type_ *types.Pointer On 2013/01/23 23:41:41, iant wrote: > Why do you need Name_ and Type_ fields here? Can't you derive them from Outer? Good point. Done. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:273: // the stack frame of the owning function. If the case where On 2013/01/23 23:41:41, iant wrote: > It's not clear to me why you discuss "the stack frame of the owning function." > In Go terms, calling a function always copies the arguments, just as with > assignment. The caller can't refer to the argument, so treating the parameter > as a pointer to the caller's argument doesn't seem meaningful. Agreed, but I think you're misreading my (confusing) use of the word "owning" to mean "calling". What I meant by owning was just the function in which this Parameter appears. I've reworded it to: "pointers into the stack frame" since it should be pretty obvious which function we mean. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:286: // (integer, fraction or complex) value. On 2013/01/23 23:41:41, iant wrote: > s/fraction/float/ (I think). Actually it can represent fractions exactly. Rounding to machine datatypes occurs later. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:544: // MakeSlice yields a slice of length Len backed by a newly allocated On 2013/01/23 23:41:41, iant wrote: > What are the values of Len and Cap if they are not specified in the program? MakeSlice corresponds to a call to the built-in function make([]T, Len) or make([]T, Len, Cap) in which Len is mandatory and Cap is assumed equal to Len if not explicitly provided. In the MakeSlice instruction, both are mandatory. I've added: // Both Len and Cap must be non-nil Values of integer type. https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:626: // TODO(adonovan): should we permit X to have type slice? On 2013/01/23 23:41:41, iant wrote: > I suppose I would permit X to have type slice. It's not clear why this should > not be permitted. After all IndexAddr followed by Load would work for an array > as well. Fair enough. I've removed the "perhaps" so this is a definite TODO, but not yet done.
Sign in to reply to this message.
Deltas since last review: - abandoned the multigraph formulation of the CFG; it's too hard to maintain. If instructions must have distinct true and false targets. - moved comparisons back from If to BinOp. If now requires a reified boolean. Also: - eliminated Name/Type fields of Builtin; replaced by canonical *types.Func object. This avoids the needs for name-based operations on built-ins. PTAL
Sign in to reply to this message.
https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:60: ImplementsMember() // dummy method to indicate the "implements" relation. On 2013/01/24 02:47:55, adonovan wrote: > On 2013/01/23 23:41:41, iant wrote: > > Is there any reason to make ImplementsMember an exported method? > > It provides explicit documentation of the implements relation. Do we have other methods that work that way? I don't find that a particularly convincing reason. The documentation is (or should be) clear, and the code can enforce the documentation, but I don't see a need for an exported method simply to add to the documentation. > Another reason to expose the ImplementsFoo method (which doesn't apply in this > case since interface Member is a "closed" type) is to permit clients to > implement the interface. Right, but as you say that doesn't apply in this case.
Sign in to reply to this message.
https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go File src/pkg/exp/ssa/ssa.go (right): https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newc... src/pkg/exp/ssa/ssa.go:60: ImplementsMember() // dummy method to indicate the "implements" relation. On 2013/01/24 18:05:54, iant wrote: > On 2013/01/24 02:47:55, adonovan wrote: > > On 2013/01/23 23:41:41, iant wrote: > > > Is there any reason to make ImplementsMember an exported method? > > > > It provides explicit documentation of the implements relation. > > Do we have other methods that work that way? I don't find that a particularly > convincing reason. The documentation is (or should be) clear, and the code can > enforce the documentation, but I don't see a need for an exported method simply > to add to the documentation. Not that I know of. I agree the documentation should be clear, but I found that it was very hard to infer the implements relation from the godoc, so I wrote explicit comments for every interface and concrete type. gri didn't like that because of the very real risk of it becoming stale and suggested I use a method instead.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=3ef1c4fabb5f *** exp/ssa: API and documentation. R=gri, iant, crawshaw, bradfitz, gri, iant CC=golang-dev https://codereview.appspot.com/7071058
Sign in to reply to this message.
|
