FAQ
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#newcode36
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#newcode39
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#newcode49
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#newcode56
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#newcode94
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#newcode95
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#newcode96
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#newcode99
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#newcode114
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#newcode129
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#newcode147
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#newcode158
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#newcode205
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#newcode244
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#newcode275
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#newcode276
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#newcode310
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#newcode314
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#newcode351
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#newcode353
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#newcode361
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#newcode390
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#newcode396
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#newcode462
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#newcode488
src/pkg/exp/ssa/ssa.go:488: Register
surprisingly litte commentary here compared to elsewhere

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

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 18, '13 at 12:13a
activeJan 18, '13 at 1:58a
posts2
users1
websitegolang.org

1 user in discussion

Gri: 2 posts

People

Translate

site design / logo © 2021 Grokbase