[PATCH] D129389: [clang][deps] Override dependency and serialized diag files for modules

[Jan Svoboda via Phabricator via cfe-commits]

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