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