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

[Robert Widmann via Phabricator via llvm-commits]

CodaFi 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88190