[PATCH] Initial version of Go bindings.

[Peter Collingbourne]

================
Comment at: bindings/go/llvm/bitwriter.cpp:25
@@ +24,3 @@
+
+LLVMMemoryBufferRef gollvm_WriteBitcodeToMemoryBuffer(LLVMModuleRef M) {
+  std::string Data;
----------------
chandlerc wrote:
> pcc wrote:
> > chandlerc wrote:
> > > Is there any particular reason to not add this to the existing C APIs?
> > > 
> > > Also, if it remains, this file should be named with CamelCase. Probably as GoBitWriter or BitWriterBindings.
> > > Is there any particular reason to not add this to the existing C APIs?
> > 
> > In general, for this change I tried to avoid modifying the C API, mainly because of the stability policy.
> > 
> > That said, this specific function looks fine to add to the C API. Done in r219643.
> > 
> > > Also, if it remains, this file should be named with CamelCase. Probably as GoBitWriter or BitWriterBindings.
> > 
> > From the point of view of a maintainer, it's convenient to be able to find the C++ for `foo.go` in `foo.cpp`. Also, the coding standards don't seem to prescribe file names. But this isn't a big deal, I'm happy to rename them.
> I think I end up on the side of consistency with other C++ code rather than consistency with the Go code. This is probably because I want to see all of the C++ code moved out of the Go bindings and into a common C interface that other bindings can use. =]
> 
> I would suggest normal PascalCase file names, and suffixing with 'Bindings' to avoid collisions with the existing files.
Done.

================
Comment at: bindings/go/llvm/dibuilder.h:10
@@ +9,3 @@
+//
+// This file defines C bindings for the DIBuilder class.
+//
----------------
chandlerc wrote:
> pcc wrote:
> > chandlerc wrote:
> > > This file too should be GoDIBuilderBindings.{h,cpp} or something CamelCased.
> > > 
> > > Do you think these should be Go-specific, or generic C APIs for bindings? I'm curious how you see this evolving over time.
> > There's no technical reason why these couldn't be part of the C API, but because of our C API stability policy, I think they should live here for now.
> > 
> > We could consider changing this policy in the future, for example by defining a core 'stable' subset of the C API, but I feel that that discussion should only happen as and when it needs to (e.g. if we add another set of language bindings that expose debug info).
> Yes, I agree and having talked to others suspect this would be a widely appreciated change. Clearly it can't happen here though.
> 
> However, I would structure the code in anticipation of that change and leave comments in the relevant .h files that these bindings shouldn't be Go-specific and should eventually move to a (somewhat) less stable collection of C APIs for use in creating bindings of LLVM in other languages.
> 
> That seem like a good path?
Sounds good, done.

================
Comment at: bindings/go/llvm/dibuilder.h:15
@@ +14,3 @@
+#ifndef GOLLVM_LLVM_DIBUILDER_H
+#define GOLLVM_LLVM_DIBUILDER_H
+
----------------
chandlerc wrote:
> I would probably name these LLVM_BINDINGS_GO_LLVM_DIBUILDERBINDINGS_H for consistency... Not a big deal, but 'GOLLVM' seems hard to intuit without knowing the history of the project. =]
Done.

================
Comment at: bindings/go/llvm/dibuilder.h:26
@@ +25,3 @@
+
+LLVMDIBuilderRef gollvm_NewDIBuilder(LLVMModuleRef m);
+
----------------
chandlerc wrote:
> The (arguably bad) LLVM convention for C-APIs has been: 'LLVMFooBarBaz'. Considering you're using 'LLVMDIBuilderRef' above, I think it makes sense to go ahead and follow this convention here. Doesn't much matter whether that lands in this patch or in a follow-up though.
Done.

================
Comment at: bindings/go/llvm/ir.go:1
@@ +1,2 @@
+//===- ir.go - Bindings for ir --------------------------------------------===//
+//
----------------
chandlerc wrote:
> I know its pedantic, but could you capitalize 'IR' when using it as a normal noun in a sentence? (Clearly the Go APIs and files need to remain lower case, just talking about the comments.)
Done.

================
Comment at: bindings/go/llvm/support.h:21-24
@@ +20,6 @@
+
+void gollvm_LoadLibraryPermanently(const char *Filename, char **ErrMsg);
+
+void gollvm_ParseCommandLineOptions(int argc, const char *const *argv,
+                                    const char *Overview);
+
----------------
chandlerc wrote:
> pcc wrote:
> > chandlerc wrote:
> > > These both seem moderately terrifying to expose in the Go bindings. Why were they needed?
> > These were needed to implement `-fload-plugin` and `-mllvm` in the Go frontend; despite the problems with them, it is sometimes useful or convenient for Go frontend users or developers to be able to set `llvm::cl` flags or load plugins from the command line. I'd imagine that if we expose command line flags through some other mechanism in the future, this API would change as well.
> I'm much more comfortable exposing the command line option parsing (until some glorious day when the -mllvm tunables can be set through a saner API). The plugin loading is somewhat concerning to me... Our plugin support is dodgy at best, and surfacing it to frontends is frankly terrifying... but hey, there you go.
> 
> I'm actually rather surprised that the plugin loading is best accomplished through LLVM's pretty terrible shared library loading wrapper rather than through native Go interfaces.
> 
> For the commandline flags, I would actually be find to put it into the C API. If it changes (when, I hope), it will just become a no-op and a new interface will be provided for setting such flags.
> I'm actually rather surprised that the plugin loading is best accomplished through LLVM's pretty terrible shared library loading wrapper rather than through native Go interfaces.

Go (the language) does not currently support dynamic loading of packages, so there is no standard library function for this. You can call dlopen directly from Go, but that isn't portable (admittedly LLVM's wrapper isn't much better, but I suppose in theory it would know what the "correct" way of loading LLVM plugins is for any given platform).

> For the commandline flags, I would actually be find to put it into the C API. If it changes (when, I hope), it will just become a no-op and a new interface will be provided for setting such flags.

Sounds good. I split this out into r219975.

================
Comment at: bindings/go/llvm/target.go:210
@@ +209,3 @@
+
+///////////////////////////////////////////////////////////////////////////////
+// Target
----------------
chandlerc wrote:
> nit: There seem to be a mixture of horizontal rule comment styles... I would pick on and be consistent with it.
> 
> But maybe there is some Go style thing going on here that I'm just not aware of. If so, ignore me. =]
Done.

http://reviews.llvm.org/D5684