[PATCH] D144058: [clang][deps] Split lookupModuleOutput out of DependencyConsumer NFC

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.


================
Comment at: clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:64-66
+/// Dependency scanner callbacks that are used during scanning to influence the
+/// behaviour of the scan - for example, to customize the scanned invocations.
+class DependencyActions {
----------------
jansvoboda11 wrote:
> What do the downstream callbacks do? The class name sounds a bit generic for something you can call `lookupModuleOutput()` on, but maybe that's the right name with more context. It's also very similar to the existing `DependencyScanningAction`.
They are callbacks that modify the ScanInstance itself, or one of the CompilerInvocations being constructed (either for the TU or for module dependencies).  In practice, we use this to add caching support - modifying invocations to use inputs from a CAS. I chose "actions" based on the fact they are acting on the scanner/invocations, rather than simply consuming the information.  While lookupModuleOutput does not directly modify the invcation, it does indirectly via the ModuleDepCollector incorporating its results into the invocation.  If you're interested, here are our downstream callbacks that I would move into this interface: https://github.com/apple/llvm-project/blob/next/clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h#L49

RE: DependencyScanningAction: I agree it's not ideal that these names are "close".  On the other hand, they're not too likely to be confused in practice since they have very different roles -- the DependencyScanningAction is not exposed to clients, and it's a tooling action not something you use to control the scan.  We also have other "actions" like the FrontendAction we create, which again have a different role.

Happy to consider suggestions for a better name!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144058