FAQ
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#newcode33
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#newcode60
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#newcode77
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#newcode142
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#newcode179
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#newcode182
src/pkg/exp/ssa/ssa.go:182: ImplementsInstruction()
Exported?

https://codereview.appspot.com/7071058/diff/17003/src/pkg/exp/ssa/ssa.go#newcode236
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#newcode261
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#newcode273
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#newcode286
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#newcode544
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#newcode626
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.

https://codereview.appspot.com/7071058/

Search Discussions

  • Adonovan at Jan 24, 2013 at 2:47 am
    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#newcode33
    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#newcode60
    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#newcode77
    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#newcode142
    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#newcode179
    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#newcode182
    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#newcode236
    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#newcode261
    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#newcode273
    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#newcode286
    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#newcode544
    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#newcode626
    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.

    https://codereview.appspot.com/7071058/
  • Adonovan at Jan 24, 2013 at 5:07 pm
    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


    https://codereview.appspot.com/7071058/
  • Iant at Jan 24, 2013 at 6:05 pm
    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#newcode60
    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.

    https://codereview.appspot.com/7071058/
  • Adonovan at Jan 24, 2013 at 7:29 pm
    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#newcode60
    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.

    https://codereview.appspot.com/7071058/
  • Iant at Jan 24, 2013 at 9:17 pm
  • Adonovan at Jan 24, 2013 at 10:21 pm
    *** 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


    https://codereview.appspot.com/7071058/

    --

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 23, '13 at 11:41p
activeJan 24, '13 at 10:21p
posts7
users2
websitegolang.org

2 users in discussion

Adonovan: 4 posts Iant: 3 posts

People

Translate

site design / logo © 2021 Grokbase