[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.


================
Comment at: clang/include/clang/Driver/Phases.h:24
+    Link,
+    IfsMerge
   };
----------------
Trailing comma please


================
Comment at: clang/lib/Driver/Driver.cpp:3341
     llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> FullPL;
-    types::getCompilationPhases(InputType, FullPL);
+    types::getCompilationPhases(
+        (Args.getLastArg(options::OPT_emit_iterface_stubs) ? types::TY_IFS
----------------
Bleh, this really should be something that we should be able to determine based on the input type.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3688
               .Case("experimental-ifs-v1", "experimental-ifs-v1")
-              .Default("");
+              .Default("experimental-ifs-v1");
 
----------------
cishida wrote:
> nit: no need to check `ArgStr` if all outcomes are the same 
Why have this switch and check below at all?  I think that it should just be an assertion that the string is equal to that value at the very most.  L3690-L3700 is dead code.  Please remove, along with L3685-L3688.


================
Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.cpp:15
+
+void tools::ifstool::Merger::ConstructJob(Compilation &C, const JobAction &JA,
+                                          const InputInfo &Output,
----------------
Why not:

```
namespace tools {
namespace ifstool {
```


================
Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.cpp:26
+  else
+    CmdArgs.push_back("write-bin");
+  CmdArgs.push_back("-o");
----------------
A ternary might be easier to read


================
Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.cpp:29
+  CmdArgs.push_back(Output.getFilename());
+  for (auto Input : Inputs)
+    CmdArgs.push_back(Input.getFilename());
----------------
`const auto &Input`?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1741
               .Case("experimental-ifs-v1", frontend::GenerateInterfaceIfsExpV1)
-              .Default(llvm::None);
+              .Default(frontend::GenerateInterfaceIfsExpV1);
       if (!ProgramAction) {
----------------
This seems entirely unnecessary, the default and case list is identical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63978