jansvoboda11 added a comment.
This looks pretty nice. My only concern is the ad-hoc command line parsing.
================
Comment at:
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:55
+ getCommandLine(llvm::function_ref<
+ Expected<const ModuleOutputOptions &>(const ModuleID &)>
+ LookupModuleOutputs) const;
----------------
I'm curious whether you have encountered situations where being able to return
an error from `LookupModuleOutputs` is useful.
================
Comment at: clang/test/ClangScanDeps/preserved-args.c:1
// RUN: rm -rf %t && mkdir %t
// RUN: cp -r %S/Inputs/preserved-args/* %t
----------------
I think we can delete this test at this point, since we check both interesting
cases in `generate-modules-path-args.c` and `removed-args.c`.
If I remember correctly, this test was originally checking that the scanner
does not change `-MT` and friends before the scan and doesn't leak that into
the resulting command lines.
================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:297
+ ? llvm::cantFail(FD.getCommandLine(
+ [&](ModuleID MID) -> const ModuleOutputOptions & {
+ return lookupModuleOutputs(MID);
----------------
Is my understanding correct that this lambda of type `const ModuleOutputOptions
&(ModuleID)` gets implicitly converted to `llvm::function_ref<Expected<const
ModuleOutputOptions &>(const ModuleID &)>`?
If so, I think it would be clearer to take the `ModuleID` by const ref here
also and wrap the return type with `Expected`, to match the... expected
`function_ref` type. WDYT?
================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:397
+ for (StringRef Arg : Args) {
+ if (Arg == "-serialize-diagnostics")
+ SerializeDiags = true;
----------------
I think we should be avoiding ad-hoc command line parsing, it can be incorrect
in many ways. Could we move this check somewhere where we have a properly
constructed `CompilerInvocation`? I think we could do something like this in
`ModuleDeps::getCanonicalCommandLine`:
```
if (!CI.getDiagnosticOpts().DiagnosticSerializationFile.empty())
CI.getDiagnosticOpts().DiagnosticSerializationFile
= LookupModuleOutput(ID, ModuleOutputKind::DiagnosticSerializationFile);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129389/new/
https://reviews.llvm.org/D129389
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
