exp/ssa: (#2 of 5): core utilities
This CL includes the implementation of Literal, all the
Value.String and Instruction.String methods, the sanity
checker, and other misc utilities.
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/sanity.go File src/pkg/exp/ssa/sanity.go (right): https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/sanity.go#newcode23 src/pkg/exp/ssa/sanity.go:23: // are written to reporter if non-nil, os.Stderr otherwise ...
12 years, 2 months ago
(2013-01-25 15:37:50 UTC)
#4
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/sanity.go
File src/pkg/exp/ssa/sanity.go (right):
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/sanity.go#ne...
src/pkg/exp/ssa/sanity.go:23: // are written to reporter if non-nil, os.Stderr
otherwise . Some
On 2013/01/25 05:33:02, iant wrote:
> s/otherwise ./otherwise./
Done.
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/sanity.go#ne...
src/pkg/exp/ssa/sanity.go:60: var buf bytes.Buffer
On 2013/01/25 05:33:02, iant wrote:
> Why do you use the bytes.Buffer here? Why not always write directly to
> s.reporter?
It's a legacy of when reporter was a callback. Changed as suggested.
(Curious: unlike C's fprintf/fputs, Fprintf/WriteString to os.Stderr are not
buffered. Does "Go style" typically use a bytes.Buffer to avoid extra write(2)
calls or just ignore this until it shows up in a profile?)
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/util.go
File src/pkg/exp/ssa/util.go (right):
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/util.go#newc...
src/pkg/exp/ssa/util.go:20: for {
On 2013/01/25 05:33:02, iant wrote:
> This function uses both a loop and a recursive call. That seems like
overkill.
> Won't it always break out of the loop the second time around?
I wrote that while coming down from an unusually heavy night on the crack-pipe.
(I was turning a tailcall into a loop but passed out halfway, leaving both.)
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/util.go#newc...
src/pkg/exp/ssa/util.go:36: case *ast.LabeledStmt:
On 2013/01/25 05:33:02, iant wrote:
> If the last statement is a label, then it would seem that there could be a
goto
> to that label, in which case the last statement in the function isn't quite a
> return statement at all. But I'm not sure what you are using this function
for.
This function is used only as part of a temporary workaround for the fact that
the typechecker doesn't yet ensure all the (non-type-related) integrity
properties such as control-flow integrity, e.g. no falling off the end of a
non-void function.
So hasReturn would (correctly) return false for this (illegal) function:
func f() int {goto L; L:}
But I found a way to implement this check on the CFG rather than the AST, so I
no longer need this function.
https://codereview.appspot.com/7199052/diff/5001/src/pkg/exp/ssa/util.go#newc...
src/pkg/exp/ssa/util.go:65: if typ, ok := typ.(*types.NamedType); ok {
On 2013/01/25 05:33:02, iant wrote:
> It's a bit subtle to use the name "typ" for both the parameter and for the
> variable in this if statement. It looks at first glance like typ will always
be
> nil in the next statement. I suggest using a different name in the if
> statement.
FWIW, this is a copy of a function in go/types which I suggest be exported.
Both gri and I make heavy use of the "x := x.(type)" trick to avoid the need to
come up with original names.
IMHO the only downside is that the inner 'typ' hides the outer so that if you
want the interface (not concrete) value again you are implicitly reconstructing
it. But it's hard for me to believe this cost is really measurable.
Issue 7199052: code review 7199052: exp/ssa: (#2 of 5): core utilities
(Closed)
Created 12 years, 2 months ago by adonovan
Modified 12 years, 1 month ago
Reviewers:
Base URL:
Comments: 17
