[PATCH] [llgo] Introduce llgoi, a REPL for Go

[Peter Collingbourne]

> I wonder if we should be leaning a little more on go/parser, so we can identify and error on labeled statements, for example. I don't think we need to worry about that just now, though.


I agree that the diagnostics could be improved in many cases. Ideally I would like llgoi to be able to do all parsing/type checking of the user input up front, and treat any errors from parsing/type checking the final package as bugs, but I'm not entirely sure how best this should be achieved.


================
Comment at: cmd/llgoi/llgoi.go:96
@@ +95,3 @@
+		GenerateDebug:  true,
+		PackageCreated: in.augmentPackageScope,
+	}
----------------
axw wrote:
> This should only apply to on-the-fly source packages, not ones on-disk. Perhaps add another flag to loadSourcePackage which determines whether or not to augment.
Ah, yes, fixed. It seemed neater for loadSourcePackage to take a copts instead.

================
Comment at: cmd/llgoi/llgoi.go:126
@@ +125,3 @@
+
+func (in *interp) Dispose() {
+	in.engine.Dispose()
----------------
axw wrote:
> lowercase, for consistency
Done.

================
Comment at: cmd/llgoi/llgoi.go:130
@@ +129,3 @@
+
+func (in *interp) loadSourcePackageFromCode(pkgcode, pkgpath string) (pkg *types.Package, err error) {
+	fset := token.NewFileSet()
----------------
axw wrote:
> Can we please follow https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters, and not named results unless it's helpful documentation-wise? I'd prefer no naked returns either. "return nil, err" makes it immediately obvious what all the result values are.
Done.

================
Comment at: cmd/llgoi/llgoi.go:282
@@ +281,3 @@
+	var code bytes.Buffer
+	code.WriteString("package ")
+	code.WriteString(pkgname)
----------------
axw wrote:
> fmt.Fprintf(&code, "package %s", pkgname)
Done.

================
Comment at: cmd/llgoi/llgoi.go:288
@@ +287,3 @@
+	for _, pkg := range in.imports {
+		code.WriteString("import \"")
+		code.WriteString(pkg.Path())
----------------
axw wrote:
> fmt.Fprintf(&code, "import %q\n", pkg.Path())
Done.

================
Comment at: cmd/llgoi/llgoi.go:312
@@ +311,3 @@
+
+		code.WriteString("var ")
+		for i := range typs {
----------------
axw wrote:
> Should this error if len(assigns)>0 and there are no new variables?
Yes it should; I felt that this was more a QOI issue so I left it out of the initial version.

================
Comment at: cmd/llgoi/llgoi.go:327
@@ +326,3 @@
+		}
+		code.WriteString(" = ")
+		code.WriteString(l.line)
----------------
axw wrote:
> fmt.Fprintf(&code, " = %s\n\n", l.line)
Done

================
Comment at: cmd/llgoi/llgoi.go:356
@@ +355,3 @@
+
+		code.WriteString("func init() {\n\t")
+		code.WriteString(l.line)
----------------
axw wrote:
> fmt.Fprintf(&code, "func init() {\n\t%s}", l.line)
> (is the whitespace doing anything useful? I guess if we want to log the program source)
Done.

Yes, the whitespace is semantically insignificant (except for semicolon insertion at newlines), but it is useful if we want to look at the source.

================
Comment at: cmd/llgoi/llgoi.go:476
@@ +475,3 @@
+		}
+		in.imports = append(in.imports, pkg)
+		return nil
----------------
axw wrote:
> Should we be making in.imports a map, to better handle duplicate imports? If you do that now, llgoi panics.
We should be handling a number of things of that nature better. As one other example, if you declare a variable with the same name as an import, llgoi gets confused and refuses to interpret any more lines. I figured this would be good enough for the initial version though.

================
Comment at: cmd/llgoi/llgoi.go:509
@@ +508,3 @@
+
+	r := bufio.NewReader(os.Stdin)
+	for {
----------------
axw wrote:
> This would be a little neater using bufio.Scanner. I guess eventually we'd want to use GNU readline, or golang.org/x/crypto/ssh/terminal, or ...
> I guess eventually we'd want to use GNU readline, or golang.org/x/crypto/ssh/terminal, or ...

Yes, or something like that. I think it would be very cool if this could do tab completion, but I couldn't find a suitable package (not to mention the details of actually computing the completions, which doesn't seem easy with the current go/types package).

http://reviews.llvm.org/D6957

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/