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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked an inline comment as done.
benlangmuir added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:55
+  getCommandLine(llvm::function_ref<
+                 Expected<const ModuleOutputOptions &>(const ModuleID &)>
+                     LookupModuleOutputs) const;
----------------
jansvoboda11 wrote:
> I'm curious whether you have encountered situations where being able to return an error from `LookupModuleOutputs` is useful.
I ended up backing this change out: it was motivated by a downstream libclang API change that I have now re-evaluated based on your other feedback to use a per-output callback instead of a single callback.


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:297
+            ? llvm::cantFail(FD.getCommandLine(
+                  [&](ModuleID MID) -> const ModuleOutputOptions & {
+                    return lookupModuleOutputs(MID);
----------------
jansvoboda11 wrote:
> 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?
This was unintentional, I just missed these couple of places when I changed the API from `ModuleID` to `const ModuleID &`.  Will fix, thanks!


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:397
+    for (StringRef Arg : Args) {
+      if (Arg == "-serialize-diagnostics")
+        SerializeDiags = true;
----------------
jansvoboda11 wrote:
> 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);
> ```
Yeah, we can do that. I originally avoided this due to it "leaking" whether the TU used these outputs into the module command-lines (since the set of callbacks would differ), but I suspect in practice that doesn't matter since you're unlikely to mix compilations that have and don't have serialized diagnostics. To be 100% sound, it will require adding the existence of the outputs to the module context hash (not the actual path, just whether there was a diag and/or d file at all). I will do the context hash change later if you're okay with it - there's nowhere to feed the extra info into `getModuleHash` right now, but I was already planning to change the hashing which will make it easier to do.  If you think it's critical we could add a parameter to `getModuleHash` temporarily to handle it.

I liked your idea to make the callback per-output as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129389