[PATCH] D63579: [clang-scan-deps] print the dependencies to stdout and remove the need to use -MD options in the CDB

[Alex Lorenz via Phabricator via cfe-commits]

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


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:32
 
+class DependencyCollectorFactory {
+public:
----------------
aganea wrote:
> arphaman wrote:
> > aganea wrote:
> > > Do you envision future uses for this factory?
> > Most likely, yes.
> > 
> > I don't want to lock-in into creating the dependency printer in the `PreprocessorOnlyTool`, as the eventual goal is to have it in a library, and allow clients to consume modular dependencies not only using `clang-scan-deps` tool itself, but using the C++ library directly (potentially through a libclang C interface as well). The printer should be in `clang-scan-deps` tool itself though, so it wouldn't be ideal to reference it from `PreprocessorOnlyTool` now.
> My initial question was, what are the future planned factories? (and thus the other `DependencyCollector` implementations?)
> 
> Do you think we could have the printer in the library as well?
> 
> On a related note - we're already testing `clang-scan-deps` compiled as a DLL. Our use-case right now is to call it from each of [[ https://github.com/fastbuild/fastbuild | Fastbuild ]]'s worker threads. We are using `main()` below almost as-is (except for the multithreading part which Fastbuild already does). The CDB JSON is passed by value from the Fastbuild thread, so it doesn't go through a file. I'd like to get back dependencies-as-a-string instead of `llvm::outs()`. The bottleneck currently is, as you can imagine, reading / writing files (seen in ETW traces), this patch should improve things a bit. I can send a sample of how we're using it after you commit.
> My initial question was, what are the future planned factories? (and thus the other DependencyCollector implementations?)

The only factory and collector right now that I have in mind is the factory that collects the dependency list from the generator, and returns them to the user of `PreprocessorOnlyTool`. Something like this:

```
class DependencyConsumer : public DependencyFileGenerator {
public:
  class Factory : public DependencyCollectorFactory {
  public:
    std::shared_ptr<DependencyCollector> createDependencyCollector(
        std::unique_ptr<DependencyOutputOptions> Opts) override {
       return DependencyConsumer(std::move(Opts));
     }
  };

  DependencyConsumer(std::unique_ptr<DependencyOutputOptions> Opts)
      : DependencyFileGenerator(*Opts), Opts(std::move(Opts)) {}

  void finishedMainFile(DiagnosticsEngine &Diags) override {
    Dependencies = getDependencies();
  }
private:
  std::vector<std::string> Dependencies;
};
```

But the current design is not ideal for supporting that actually. I think I'll rework it following the suggested SharedStream you mentioned earlier.

> Do you think we could have the printer in the library as well?

Yes

> On a related note - we're already testing clang-scan-deps compiled as a DLL. Our use-case right now is to call it from each of Fastbuild's worker threads. We are using main() below almost as-is (except for the multithreading part which Fastbuild already does). 

Nice! I think the library will be a great fit for you then, the plan is to have a service object in it which will own the shared state (like the cache of the minimized sources), and worker objects which you can use in each thread to do the actual work.


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

https://reviews.llvm.org/D63579