[PATCH] D60233: [clang-scan-deps] initial outline of the tool that runs preprocessor to find dependencies over a JSON compilation database

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.


================
Comment at: tools/clang-scan-deps/ClangScanDeps.cpp:39
+  bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
+                     FileManager *Files,
+                     std::shared_ptr<PCHContainerOperations> PCHContainerOps,
----------------
`Files` -> `FileMgr` might be more readable.


================
Comment at: tools/clang-scan-deps/ClangScanDeps.cpp:56
+    auto Action = llvm::make_unique<PreprocessOnlyAction>();
+    const bool Success = Compiler.ExecuteAction(*Action);
+    Files->clearStatCache();
----------------
`Success` -> `Result`?


================
Comment at: tools/clang-scan-deps/ClangScanDeps.cpp:91
+public:
+  DependencyScanningTool(const tooling::CompilationDatabase &Compilations)
+      : Compilations(Compilations) {
----------------
Maybe it's obvious but maybe we could add some mention of expected lifetime of `Compilations` argument.


================
Comment at: tools/clang-scan-deps/ClangScanDeps.cpp:112
+  std::shared_ptr<PCHContainerOperations> PCHContainerOps;
+  /// The real filesystem used a base for all the operations performed by the
+  /// tool.
----------------
Missing "as"?


================
Comment at: tools/clang-scan-deps/ClangScanDeps.cpp:124
+    NumThreads("j", llvm::cl::Optional,
+               llvm::cl::desc("Number of worker threads to use"),
+               llvm::cl::init(0));
----------------
We might mention the default value (`llvm::hardware_concurrency()`).


================
Comment at: tools/clang-scan-deps/ClangScanDeps.cpp:203
+            // Run the tool on it.
+            HadErrors = WorkerTools[I]->runOnFile(Input, CWD);
+          }
----------------
Shouldn't this be `|=`? This seems like we're returning only the result of the last run.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60233