[PATCH] D94461: [llvm-ifs] Add option to use InterfaceStub library

[Haowei Wu via Phabricator via llvm-commits]

haowei added inline comments.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:58
 
+static cl::opt<bool> UseInterfaceStub(
+    "use-interfacestub",
----------------
plotfi wrote:
> Put a comment here to the effect that the InterfaceStubs backend is newer than "Clang InterfaceStubs" which originally used the yaml2obj backend. Eventually I assume we want to flip the default for this too. Can also you explicitly set the default here to make that clear? 
I slightly modified the flag desc, is that enough? Also I explicitly set the default to false.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:552
 
+  if (UseInterfaceStub && (Action == "write-bin")) {
+    elfabi::ELFStub ElfStub;
----------------
plotfi wrote:
> What happens if UseInterfaceStub  == true but Action is not "write-bin" ? Shouldn't it generate a merged llvm-elftapi text file or something like that or at least error out? 
In diff 1, when `UseInterfaceStub == true but Action is not "write-bin"`, it will fallback to function writeIfso function, which will generate an ifs text file correctly. But I think it miss the case when `UseInterfaceStub == true` but output format is `TBD`, which will output nothing.

In diff3, InterfaceStub will only be used when `UseInterfaceStub == true && Action == "write-bin" && Format != "TBD"`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94461