[PATCH] D60974: Clang IFSO driver action.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 3 inline comments as done.
plotfi added a comment.

In D60974#1534662 <https://reviews.llvm.org/D60974#1534662>, @rupprecht wrote:

> Can you upload this patch with context? Either use arc or upload w/ -U99999


Thanks for the review.

> I seem to have a lot of comments, but they're all nits -- my major concern being the `llvm_unreachable`s should be errors instead (i.e. should be triggered in release builds).
> 
> Since this is clearly labeled as experimental, I think you should feel free to commit if you can get another lgtm (@compnerd?)





================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3614-3616
+              Args.hasArg(options::OPT_iterface_stub_version_EQ)
+                  ? Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)
+                  : "")
----------------
rupprecht wrote:
> `Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)` should already default to returning the empty string if unset (note the default param)
But what if it is set to something that's not either experimental-yaml-elf-v1 or experimental-tapi-elf-v1? This was to truncate any values that aren't those two to "" so that the error check could just be if (StubFormat.empty()). Maybe something other than a string switch would have been more explicit. 


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1688
+      if (!ProgramActionPair.second)
+        llvm_unreachable("Must specify a valid interface stub format.");
+      Opts.ProgramAction = ProgramActionPair.first;
----------------
rupprecht wrote:
> I think this is very much reachable if someone provides `--interface-stub-version=x`
AH yes, you are right. This should probably be a diag. (clang/test/InterfaceStubs/bad-format.cpp tests for this "unreachable").


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:41
+      return true;
+    if (Symbols.find(ND) != Symbols.end())
+      return true;
----------------
rupprecht wrote:
> llvm::is_contained(Symbols, ND)
llvm::is_contained does not appear to work with std::map. I will try to figure out why. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974