[PATCH] Initial version of Go bindings.
Rui Ueyama
ruiu at google.com
Fri Oct 10 17:05:56 PDT 2014
Looks like a straightforward LLVM binding for Go.
Here's my review comments from the perspective of a Go programmer.
================
Comment at: bindings/go/llvm/bitreader.go:49
@@ +48,3 @@
+ C.free(unsafe.Pointer(errmsg))
+ return Module{nil}, err
+}
----------------
At line 38 you return Module{}, err. Here you return Module{nil}, err. Are these different?
================
Comment at: bindings/go/llvm/dibuilder.go:26
@@ +25,3 @@
+const (
+ LLVMDebugVersion = (12 << 16)
+)
----------------
What is this number?
================
Comment at: bindings/go/llvm/dibuilder.go:176
@@ +175,3 @@
+
+func (d *DIBuilder) CreateLexicalBlockFile(diScope Value, diFile Value,
+ discriminator int) Value {
----------------
Do you follow 80 column rule even in Go? We don't have the Go coding style in LLVM, but are you going to set it?
================
Comment at: bindings/go/llvm/executionengine.go:40
@@ +39,3 @@
+ }
+)
+
----------------
I prefer writing
type GenericValue struct {
...
}
type ExecutionEngine struct {
...
}
instead of grouping them in the type.
================
Comment at: bindings/go/llvm/executionengine.go:87
@@ +86,3 @@
+ } else {
+ err = nil
+ }
----------------
You don't need this line
================
Comment at: bindings/go/llvm/executionengine.go:100
@@ +99,3 @@
+ } else {
+ err = nil
+ }
----------------
Ditto
================
Comment at: bindings/go/llvm/executionengine.go:119
@@ +118,3 @@
+ } else {
+ err = nil
+ }
----------------
Ditto
================
Comment at: bindings/go/llvm/executionengine.go:173
@@ +172,2 @@
+
+// vim: set ft=go:
----------------
Remove
================
Comment at: bindings/go/llvm/string.go:107
@@ +106,2 @@
+
+// vim: set ft=go :
----------------
Remove
================
Comment at: bindings/go/llvm/support.go:53
@@ +52,2 @@
+ C.free(unsafe.Pointer(overviewstr))
+}
----------------
Throughout this patch, you seems to avoid using defer where possible. Is there any reason behind that decision? It seems to me that this function could be written like this
for i, arg := range args {
argstr[i] = C.CString(arg)
defer C.free(unsafe.Pointer(argstr[i]))
}
and
overviewstr := C.CString(overview)
defer C.Free(unsafe.Pointer(ovewviewstr))
================
Comment at: bindings/go/llvm/transforms_pmbuilder.go:22
@@ +21,3 @@
+type (
+ PassManagerBuilder struct {
+ C C.LLVMPassManagerBuilderRef
----------------
type PassManagerBuilder struct {
http://reviews.llvm.org/D5684
More information about the llvm-commits
mailing list