[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