The gathered dependencies are now printed to the STDOUT instead of being written to disk. The tool also doesn't need the dependency output options as well to produce the dependencies.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| clang/tools/clang-scan-deps/ClangScanDeps.cpp | ||
|---|---|---|
| 32 ↗ | (On Diff #205697) | Do you envision future uses for this factory? |
| 50 ↗ | (On Diff #205697) | Do you need the blocking behavior here? You're simply passing the Lock by reference, but not using the stream that it protects. |
| 73 ↗ | (On Diff #205697) | I think it would be better to keep together the lock with what it protects (thus my question above about the factory). 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? |
| 115 ↗ | (On Diff #205697) | ..and in that case here we would say: Compiler.addDependencyCollector(std::make_shared<DependencyPrinter>(DepOpts, SharedOS)) |
| 245 ↗ | (On Diff #205697) | ...and then here: SharedStream S(llvm::outs);. |
Comment Actions
removed the lock
| clang/tools/clang-scan-deps/ClangScanDeps.cpp | ||
|---|---|---|
| 32 ↗ | (On Diff #205697) | 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. |
| 50 ↗ | (On Diff #205697) | Right, I don't need it here. removed in the updated patch. |
| clang/tools/clang-scan-deps/ClangScanDeps.cpp | ||
|---|---|---|
| 32 ↗ | (On Diff #205697) | 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 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. |
| clang/tools/clang-scan-deps/ClangScanDeps.cpp | ||
|---|---|---|
| 32 ↗ | (On Diff #205697) |
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.
Yes
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. |