[PATCH] D60974: Clang IFSO driver action.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a reviewer: rupprecht.
rupprecht added a comment.

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

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)
+                  : "")
----------------
`Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)` should already default to returning the empty string if unset (note the default param)


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1678-1680
+              Args.hasArg(OPT_iterface_stub_version_EQ)
+                  ? Args.getLastArgValue(OPT_iterface_stub_version_EQ)
+                  : "")
----------------
(same here)


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1688
+      if (!ProgramActionPair.second)
+        llvm_unreachable("Must specify a valid interface stub format.");
+      Opts.ProgramAction = ProgramActionPair.first;
----------------
I think this is very much reachable if someone provides `--interface-stub-version=x`


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:20-21
+  CompilerInstance &Instance;
+  StringRef InFile = "";
+  StringRef Format = "";
+  std::set<std::string> ParsedTemplates;
----------------
nit: these default initializations are redundant


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:26
+  struct MangledSymbol {
+    std::string ParentName = "";
+    uint8_t Type;
----------------
ditto


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:39-41
+    if (!(RDO & FromTU))
+      return true;
+    if (Symbols.find(ND) != Symbols.end())
----------------
Can you add a comment why these two are excluded?


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:41
+      return true;
+    if (Symbols.find(ND) != Symbols.end())
+      return true;
----------------
llvm::is_contained(Symbols, ND)


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:44-47
+    if (isa<FieldDecl>(ND))
+      return true;
+    if (isa<ParmVarDecl>(ND))
+      return true;
----------------
This would be terser (and more readable) combined; `if (isa<FieldDecl>(ND) || isa<ParamValDecl>(ND))`


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:95
+      index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+      auto MangledNames = CGNameGen.getAllManglings(ND);
+      if (MangledNames.size() == 1)
----------------
auto -> std::vector<std::string>, unless the return type is obvious to anyone that commits here (i.e. not me)


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:107
+      index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+      auto MangledName = CGNameGen.getName(ND);
+      auto MangledNames = CGNameGen.getAllManglings(ND);
----------------
auto -> std::string


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:206-207
+    // catch anything that's not a VarDecl or Template/FunctionDecl.
+    llvm_unreachable("clang -emit-iterface-stubs: Expected a function or "
+                     "function template decl.");
+    return false;
----------------
This should be an error, not llvm_unreachable (which I think gets filtered out in release builds)


================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:242
+    MangledSymbols Symbols;
+    auto OS = Instance.createDefaultOutputFile(/*Binary=*/false, InFile, "ifo");
+    if (!OS)
----------------
elsewhere it looks like this was changed to "ifs"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974