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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
