[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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88190