[PATCH] D88190: C API: functions to get mask of a ShuffleVector

Sat Sep 26 12:23:27 PDT 2020

cdisselkoen added inline comments.

================
Comment at: llvm/lib/IR/Core.cpp:3966
+}
+const int LLVMUndefMaskElem =
+    -1; // not actually accessible as ShuffleVectorInst::UndefMaskElem, so we
----------------
CodaFi wrote:
> CodaFi wrote:
> > cdisselkoen wrote:
> > > CodaFi wrote:
> > > > cdisselkoen wrote:
> > > > > CodaFi wrote:
> > > > > > Can we add a getter for this instead? We should avoid magic numbers in the C API - it doesn’t tend to stay in sync with the C++ otherwise
> > > > > Actually, as I was looking into this, I realized that we can just write `const int LLVMUndefMaskElem = UndefMaskElem;` here and it works.  `UndefMaskElem` is just a constant available in the `Instructions.h` header.
> > > > > 
> > > > > Since this has already landed on master, should I open a new diff with this change?  Or still modify this diff?
> > > > To both questions 
> > > > 
> > > > 1) Please don’t mix C and C++ headers. It may happen to work for you locally now, but there’s no guarantee Instructions.h won’t break everybody in the future. Please make an explicit getter.
> > > > 
> > > > 2) You should submit another revision for review here.
> > > > Please don't mix C and C++ headers ... there's no guarantee Instructions.h won't break everybody in the future
> > > 
> > > Apologies for my lack of knowledge (C++ is not my primary language), but could you explain what the concern is here?  Instructions.h (and other C++ headers) are already included in Core.cpp, so the line above works without including any new headers.  And we wouldn't have to change Core.h which is the actual C header -- in that header the current diff just has a declaration `extern const int LLVMUndefMaskElem;`, and we could keep that declaration the same.
> > The C bindings are meant to present a pure C view of a C++ API.  It is only natural then that we *implement* these bindings in C++, but we express the interface to those bindings in C. But despite their names these languages are strictly incompatible. If I were to build a C-only translation unit with a C-only compiler that imported Instructions.h, I would not succeed. There are clients like the OCaml bindings, go bindings, and my own set of Swift bindings that would also break upon trying to import a C++ header in a C context.
> The C API also has additional stability commitments that the C++ API roundly does not make. In order to carry out those guarantees, we make heavy use of abstraction. Notice that every API here that takes LLVM objects takes them in memory through bare typedefs of opaque pointers. Here, we have an implementation detail of the C++ API leaking to clients through this constant. If tomorrow somebody were to add support for a host that used some other value for this constant, we would be hosed by the lack of abstraction here.
> The C bindings are meant to present a pure C view of a C++ API ... If I were to build a C-only translation unit with a C-only compiler that imported Instructions.h, I would not succeed. ... OCaml bindings, go bindings, and my own set of Swift bindings would also break

Sure, totally understood on this.  (I also maintain [these Rust bindings for LLVM which depend on the C API](https://github.com/cdisselkoen/llvm-ir).)  What I don't understand is what about this change would require C consumers to import Instructions.h.  Is that somehow implied by the use of `extern const int` in the C header?  The code seems to compile for me (and pass tests) without importing Instructions.h in the C header - we're only referencing Instructions.h in the C++ implementation of the bindings.

> If tomorrow somebody were to add support for a host that used some other value for this constant

Is Core.cpp target-specific?  I don't understand how different hosts could have different values for this constant.  Again, I'm not the most familiar with C++ so this is probably me misunderstanding something.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88190/new/

https://reviews.llvm.org/D88190