code review 7199052: exp/ssa: (#2 of 5): core utilities

src/pkg/exp/ssa/sanity.go

 package ssa
 
 // An optional pass for sanity checking invariants of the SSA representation.
 // Currently it checks CFG invariants but little at the instruction level.
 
 import (
         "bytes"
         "fmt"
         "io"
         "os"
-        "reflect"
 )
 
 type sanity struct {
         reporter io.Writer
         fn       *Function
         block    *BasicBlock
         insane   bool
 }
 
 // SanityCheck performs integrity checking of the SSA representation
 // of the function fn and returns true if it was valid.  Diagnostics
-// are written to reporter if non-nil, os.Stderr otherwise .  Some

[iant, 2013/01/25 05:33:02]

s/otherwise ./otherwise./

[adonovan, 2013/01/25 15:37:51]

Done.

+// are written to reporter if non-nil, os.Stderr otherwise.  Some
 // diagnostics are only warnings and do not imply a negative result.
 //
 // Sanity checking is intended to facilitate the debugging of code
 // transformation passes.
 //
 func SanityCheck(fn *Function, reporter io.Writer) bool {
         if reporter == nil {
                 reporter = os.Stderr
         }
         return (&sanity{reporter: reporter}).checkFunction(fn)
@@ skipping 16 matching lines @@
         for i, b := range blocks {
                 if i > 0 {
                         io.WriteString(&buf, ", ")
                 }
                 io.WriteString(&buf, b.Name)
         }
         return buf.String()
 }
 
 func (s *sanity) diagnostic(prefix, format string, args ...interface{}) {
-        var buf bytes.Buffer

[iant, 2013/01/25 05:33:02]

Why do you use the bytes.Buffer here?  Why not always write directly to
s.reporter?

[adonovan, 2013/01/25 15:37:51]

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?)

[iant, 2013/01/25 16:40:49]

A bit of both, I suppose.  (In C stderr is unbuffered by default too, stdout is
line buffered.)

-        fmt.Fprintf(&buf, "%s: function %s", prefix, s.fn.FullName())
+        fmt.Fprintf(s.reporter, "%s: function %s", prefix, s.fn.FullName())
         if s.block != nil {
-                fmt.Fprintf(&buf, ", block %s", s.block.Name)
-        }
-        io.WriteString(&buf, ": ")
-        fmt.Fprintf(&buf, format, args...)
-
-        io.WriteString(s.reporter, buf.String())
+                fmt.Fprintf(s.reporter, ", block %s", s.block.Name)
+        }
+        io.WriteString(s.reporter, ": ")
+        fmt.Fprintf(s.reporter, format, args...)
         io.WriteString(s.reporter, "\n")
 }
 
 func (s *sanity) errorf(format string, args ...interface{}) {
         s.insane = true
         s.diagnostic("Error", format, args...)
 }
 
 func (s *sanity) warnf(format string, args ...interface{}) {
         s.diagnostic("Warning", format, args...)
@@ skipping 25 matching lines @@
                 s.errorf("control flow instruction not at end of block")
         case *Phi:
                 if idx == 0 {
                         // It suffices to apply this check to just the first phi node.
                         if dup := findDuplicate(s.block.Preds); dup != nil {
                                 s.errorf("phi node in block with duplicate predecessor %s", dup.Name)
                         }
                 } else {
                         prev := s.block.Instrs[idx-1]
                         if _, ok := prev.(*Phi); !ok {
-                                s.errorf("Phi instruction follows a non-Phi: %s", reflect.TypeOf(prev))
+                                s.errorf("Phi instruction follows a non-Phi: %T", prev)
                         }
                 }
                 if ne, np := len(instr.Edges), len(s.block.Preds); ne != np {
                         s.errorf("phi node has %d edges but %d predecessors", ne, np)
                 }
 
         case *Alloc:
         case *Call:
         case *BinOp:
         case *UnOp:
@@ skipping 15 matching lines @@
         case *Send:
         case *Store:
         case *MapUpdate:
         case *Next:
         case *Lookup:
         case *Conv:
         case *ChangeInterface:
         case *MakeInterface:
                 // TODO(adonovan): implement checks.
         default:
-                panic("Unknown instruction type: " + reflect.TypeOf(instr).String())
+                panic(fmt.Sprintf("Unknown instruction type: %T", instr))
         }
 }
 
 func (s *sanity) checkFinalInstr(idx int, instr Instruction) {
         switch instr.(type) {
         case *If:
                 if nsuccs := len(s.block.Succs); nsuccs != 2 {
                         s.errorf("If-terminated block has %d successors; expected 2", nsuccs)
                         return
                 }
@@ skipping 97 matching lines @@
                 if b == nil {
                         s.warnf("nil *BasicBlock at f.Blocks[%d]", i)
                         continue
                 }
                 s.checkBlock(b, i == 0)
         }
         s.block = nil
         s.fn = nil
         return !s.insane
 }