[PATCH] Initial version of Go bindings.

Peter Collingbourne peter at pcc.me.uk
Mon Oct 13 14:37:30 PDT 2014


================
Comment at: bindings/go/llvm/bitreader.go:49
@@ +48,3 @@
+	C.free(unsafe.Pointer(errmsg))
+	return Module{nil}, err
+}
----------------
ruiu wrote:
> At line 38 you return Module{}, err. Here you return Module{nil}, err. Are these different?
No. Changed here and elsewhere to be consistent.

================
Comment at: bindings/go/llvm/dibuilder.go:26
@@ +25,3 @@
+const (
+	LLVMDebugVersion = (12 << 16)
+)
----------------
ruiu wrote:
> What is this number?
It appears to refer to a set of debug info version constants that are no longer used. Removed (and sent D5760 to remove the constants).

================
Comment at: bindings/go/llvm/dibuilder.go:176
@@ +175,3 @@
+
+func (d *DIBuilder) CreateLexicalBlockFile(diScope Value, diFile Value,
+	discriminator int) Value {
----------------
ruiu wrote:
> 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?
To me it seems reasonable to follow the Go coding conventions for Go code in LLVM -- there doesn't seem to be any compelling reason to define our own conventions. I've sent out D5761 which formalizes this.

In this particular case we probably don't need the line break; removed.

================
Comment at: bindings/go/llvm/executionengine.go:40
@@ +39,3 @@
+	}
+)
+
----------------
ruiu wrote:
> I prefer writing
> 
>   type GenericValue struct {
>     ...
>   }
> 
>   type ExecutionEngine struct {
>     ...
>   }
> 
> instead of grouping them in the type.
Done.

================
Comment at: bindings/go/llvm/executionengine.go:87
@@ +86,3 @@
+	} else {
+		err = nil
+	}
----------------
ruiu wrote:
> You don't need this line
Removed.

================
Comment at: bindings/go/llvm/executionengine.go:100
@@ +99,3 @@
+	} else {
+		err = nil
+	}
----------------
ruiu wrote:
> Ditto
Ditto

================
Comment at: bindings/go/llvm/executionengine.go:119
@@ +118,3 @@
+	} else {
+		err = nil
+	}
----------------
ruiu wrote:
> Ditto
Ditto

================
Comment at: bindings/go/llvm/executionengine.go:173
@@ +172,2 @@
+
+// vim: set ft=go:
----------------
ruiu wrote:
> Remove
Done.

================
Comment at: bindings/go/llvm/string.go:107
@@ +106,2 @@
+
+// vim: set ft=go :
----------------
ruiu wrote:
> Remove
Done.

================
Comment at: bindings/go/llvm/support.go:53
@@ +52,2 @@
+	C.free(unsafe.Pointer(overviewstr))
+}
----------------
ruiu wrote:
> 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))
> 
That certainly helps with conciseness here; done.

I was not the original author of most of this code, but I agree that it would be useful (and more idiomatic) to start using defer. I went through the code to make it use defer where it made sense.

================
Comment at: bindings/go/llvm/transforms_pmbuilder.go:22
@@ +21,3 @@
+type (
+	PassManagerBuilder struct {
+		C C.LLVMPassManagerBuilderRef
----------------
ruiu wrote:
> type PassManagerBuilder struct {
Done.

http://reviews.llvm.org/D5684






More information about the llvm-commits mailing list