[PATCH] D13761: [llgo] llgoi: separate evaluation from printing

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: cmd/llgoi/llgoi.go:54
@@ +53,3 @@
+func init() {
+	flag.BoolVar(&dumpSource, "fdump-trace", false, "enable dumping source to stderr before execution")
+}
----------------
I normally just hacked this in manually whenever I needed it. Would that not work for you?

Nit: can the variable be named like the flag name?

================
Comment at: cmd/llgoi/llgoi.go:182
@@ +181,3 @@
+		symbolName := irgen.ManglePackagePath(pkgpath) + "." + name
+		global := module.Module.NamedGlobal(symbolName)
+		return in.engine.PointerToGlobal(global)
----------------
Wouldn't this return null if the source package isn't a `interpretLine`-generated package? How do things work in that case?

================
Comment at: cmd/llgoi/llgoi.go:200
@@ +199,3 @@
+	var results []interface{}
+	llgoiResultsAddress := getPackageSymbol("__llgoiResults$descriptor")
+	if llgoiResultsAddress != nil {
----------------
I don't much like this conspiracy between `loadSourcePackage` and `interpretLine`. Can we maybe have this function return `getPackageSymbol` instead of `result` and make `interpretLine` responsible for retrieving the results?

================
Comment at: cmd/llgoi/llgoi.go:531
@@ +530,3 @@
+	// package qualifiers for types defined within the interpreter.
+	fmt.Fprintf(w, "%+v", v)
+}
----------------
I was working on a pretty printing package for this, mainly because I wanted more whitespace than what `%#v` provides, but I suppose it could also be used to hide internal names. If that sounds useful at all I'll see if I can dig it up.

================
Comment at: test/llgoi/maps.test:10
@@ -8,5 +9,3 @@
 
 _, _ := m[0]
 
----------------
Here (and in a few other places) you are removing CHECK lines instead of replacing them with something equivalent. In this particular case the expression line isn't really testing much any more (not even the parser/type checker, as you aren't testing for absence of errors). I'd prefer to keep the checks unless you have a reason not to, or otherwise remove lines like this.


http://reviews.llvm.org/D13761