[PATCH] D70274: [clang][IFS] Driver pipeline change for clang-ifs: generate interface stubs after standard pipeline.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:3493
+    llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> &PL = PhaseList;
+    types::getCompilationPhases(types::TY_IFS_CPP, PL);
+    llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> CompilePhaseList;
----------------
The reference binding is odd.  Why not just pass `PhaseList`?


================
Comment at: clang/lib/Driver/Driver.cpp:3500
+      PL = CompilePhaseList;
+    }
+
----------------
Do you use the `PhaseList` again?  Why not `erase_if`?


================
Comment at: clang/lib/Driver/Driver.cpp:3506
+      types::ID InputType = I.first;
+      const Arg *InputArg = I.second;
+
----------------
structured bindings ... so much.


================
Comment at: clang/lib/Driver/Driver.cpp:3508
+
+      if (InputType == types::TY_IFS ||
+          InputType == types::TY_PP_Asm ||
----------------
A comment explaining that we cannot inspect assembly yet and why ifs don't make sense would be nice


================
Comment at: clang/lib/Driver/Driver.cpp:3515
+
+      for (phases::ID Phase : PL) {
+        if (Phase == phases::IfsMerge) {
----------------
`auto` would be fine too


================
Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.cpp:34
+    return Filename;
+  };
+
----------------
IIRC, `llvm::sys::path::replace_ext` or `stem` should do this?


================
Comment at: clang/lib/Driver/ToolChains/InterfaceStubs.cpp:45
+    OutputFilename =
+        TrimFileExt(OutputFilename, ".so") + (WriteBin ? ".ifso" : ".ifs");
+  CmdArgs.push_back(Args.MakeArgString(OutputFilename.c_str()));
----------------
`llvm::sys::fs::replace_ext`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70274