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