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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:32
 
+class DependencyCollectorFactory {
+public:
----------------
Do you envision future uses for this factory?


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:50
+        std::unique_ptr<DependencyOutputOptions> Opts) override {
+      std::unique_lock<std::mutex> LockGuard(SharedLock);
+      return std::make_shared<DependencyPrinter>(std::move(Opts), SharedLock,
----------------
Do you need the blocking behavior here? You're simply passing the Lock by reference, but not using the stream that it protects.


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:73
+  std::unique_ptr<DependencyOutputOptions> Opts;
+  std::mutex &Lock;
+  raw_ostream &OS;
----------------
I think it would be better to keep together the lock with what it protects (thus my question above about the factory).
What about:
```
class SharedStream {
public:
  SharedStream(raw_ostream &OS) : OS(OS) {}
  template <typename Fn> void applyLocked(Fn f) {
    std::unique_lock<std::mutex> LockGuard(Lock);
    f(OS);
    OS.flush();
  }

private:
  std::mutex Lock;
  raw_ostream &OS;
};

/// Prints out all of the gathered dependencies into one output stream instead
/// of using the output dependency file.
class DependencyPrinter : public DependencyFileGenerator {
public:
  DependencyPrinter(std::unique_ptr<DependencyOutputOptions> Opts,
                    SharedStream &S)
      : DependencyFileGenerator(*Opts), Opts(std::move(Opts)), S(S) {}

  void finishedMainFile(DiagnosticsEngine &Diags) override {
    S.applyLocked([](raw_ostream &OS) { outputDependencyFile(OS); });
  }

private:
  std::unique_ptr<DependencyOutputOptions> Opts;
  SharedStream &S;
};
```
WDYT?


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:115
+      Opts->Targets = {"clang-scan-deps dependency"};
+    Compiler.addDependencyCollector(
+        DepCollectorFactory.createDependencyCollector(std::move(Opts)));
----------------
..and in that case here we would say: `Compiler.addDependencyCollector(std::make_shared<DependencyPrinter>(DepOpts, SharedOS))`


================
Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:245
+  // Print out the dependency results to STDOUT by default.
+  DependencyPrinter::Factory DepPrintFactory(llvm::outs());
   unsigned NumWorkers =
----------------
...and then here: `SharedStream S(llvm::outs);`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63579