[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