This is an archive of the discontinued LLVM Phabricator instance.

[clang][deps] Split lookupModuleOutput out of DependencyConsumer NFC
ClosedPublic

Authored by benlangmuir on Feb 14 2023, 4:22 PM.

Details

Summary

The idea is to split the callbacks that are used to consume dependency
information (DependencyConsumer) from callbacks that modify the scan
behaviour itself in any way (DependencyActions). Currently this is just
lookupModuleOutput, but we have additional callbacks related to CAS
support that we intend to upstream in the future.

Diff Detail

Event Timeline

benlangmuir created this revision.Feb 14 2023, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 4:22 PM
benlangmuir requested review of this revision.Feb 14 2023, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 4:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 added inline comments.Feb 14 2023, 4:37 PM
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
64–66

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.

benlangmuir added inline comments.Feb 14 2023, 4:57 PM
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
64–66

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!

jansvoboda11 accepted this revision.Feb 15 2023, 12:53 PM
jansvoboda11 added inline comments.
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
64–66

Thanks, that's helpful. My preference would be slightly towards something more specific-sounding, maybe ScanningAdjuster, ScannerAndDependencyAdjuster, but LGTM as-is too.

This revision is now accepted and ready to land.Feb 15 2023, 12:53 PM
This revision was landed with ongoing or failed builds.Mar 10 2023, 1:15 PM
This revision was automatically updated to reflect the committed changes.