[PATCH] Initial contribution of llgo, a Go frontend

Rui Ueyama ruiu at google.com
Thu Nov 20 18:50:30 PST 2014


So far it looks pretty good. First round of review.

These comments are mostly nitpicky stuff, and I don't think we want to address them in this patch.If we do, this patch will become a port from the git repostioy plus various different kind of fixes. I left these comments just because I noticed them and didn't want to lose it. If something needs to be addressed, I think it's better to fix in a follow-up patch.

================
Comment at: tools/llgo/CMakeLists.txt:44
@@ +43,3 @@
+  DEPENDS libgo ${CMAKE_BINARY_DIR}/bin/llgo${CMAKE_EXECUTABLE_SUFFIX}
+  GOFLAGS "cc=${CMAKE_BINARY_DIR}/bin/clang"
+          "cxx=${CMAKE_BINARY_DIR}/bin/clang++"
----------------
Why do you use locally built clang to build go? Can't it compile with system's C/C++ compiler?

================
Comment at: tools/llgo/build/context.go:65
@@ +64,3 @@
+		{"openbsd.*", "openbsd"},
+	}
+	match := func(list []REs, s string) string {
----------------
The regular expressions were written as if they are surrounded by ^ and $, but they are actually not. As a result they look odd -- the trailing .* has no effect, and /k?/ in /k?freebsd/ doesn't have any effect.

================
Comment at: tools/llgo/cmd/cc-wrapper/main.go:51
@@ +50,3 @@
+	}
+}
+
----------------
os.StartProcess is a low-level interface, and for this function it doesn't seems to be needed. We should just use os.Command instead. We may be able to replace this function with a few lines of os.Command calls.

================
Comment at: tools/llgo/cmd/gllgo/gllgo.go:49
@@ +48,3 @@
+	fmt.Printf("llgo version %s (%s)\n", llvmVersion(), irgen.GoVersion())
+	fmt.Println()
+	os.Exit(0)
----------------
nit: just add one more \n to the last Printf instead of calling Println?

================
Comment at: tools/llgo/cmd/gllgo/gllgo.go:241
@@ +240,3 @@
+				return opts, errors.New("missing path after '-I'")
+			}
+			opts.importPaths = append(opts.importPaths, args[1])
----------------
So you check for errors for -I but not for -D and other options?

================
Comment at: tools/llgo/cmd/gllgo/gllgo.go:493
@@ +492,3 @@
+func getDataInlineAsm(data []byte) string {
+	edata := make([]byte, len(data)*4+10)
+
----------------
This function could be simplified if you start with the empty byte array with extra buffer (make([]byte, 0, len(data)*4+10) and append strings using append.

================
Comment at: tools/llgo/debug/debug.go:387
@@ +386,3 @@
+func (d *DIBuilder) descriptorMap(t *types.Map, name string) llvm.Value {
+	return d.descriptorBasic(types.Typ[types.Uintptr], name)
+}
----------------
Is this TODO?

================
Comment at: tools/llgo/debug/debug.go:391
@@ +390,3 @@
+func (d *DIBuilder) descriptorChan(t *types.Chan, name string) llvm.Value {
+	return d.descriptorBasic(types.Typ[types.Uintptr], name)
+}
----------------
ditto

http://reviews.llvm.org/D6327






More information about the llvm-commits mailing list