[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In general, I think this is looking pretty close to good. Whether clang-tidy should get this functionality in this form or not is a bit less clear to me. *I* think it's helpful functionality and the current approach is reasonable, but I think it might be worthwhile to have a community RFC to see if others agree. CC @alexfh in case he wants to make a code owner decision instead.



================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:321-324
+    if (MultipassPhase == MultipassProjectPhase::Collect)
+      // Allow the checks to write their data at the end of execution.
+      for (auto &Check : Checks)
+        Check->runPostCollect();
----------------
I'd add the braces for readability despite them not being strictly required.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:347-348
+                                  ClangTidyCheckFactories &CheckFactories) {
+  CheckVec Checks = CheckFactories.createChecks(&Context);
+  return Checks;
+}
----------------



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:54
+  case MultipassProjectPhase::Compact:
+    llvm_unreachable("AST Matchers should not have run in compact mode.");
+  }
----------------
I'm on the fence about whether this should be an unreachable or an assert. This seems like it's an assertion situation (if you reach that case, the programmer made a mistake; it's not that you can never reach this case).


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:118-119
+
+  /// Checks that performed ``collect`` should write their data to the output to
+  /// the file in here.
+  virtual void postCollect(StringRef OutFile) {}
----------------



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:122
+
+  /// Execute the ``postCollect()`` function to the right, automatically
+  /// generated filename.
----------------
I'm not certain what "to the right" means in this context, so some extra clarification here would be reasonable. I think it's saying that it's a post order traversal?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:242-243
+  MultipassProjectPhase Phase = getGlobalOptions().MultipassPhase;
+  if (Phase == MultipassProjectPhase::Collect)
+    llvm_unreachable("Invalid phase 'Collect' for accessing compacted data");
+
----------------
I think this would be expressed better via an `assert`.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:246
+  auto InsertResult = CompactedDataPaths.try_emplace(CheckName, "");
+  std::string &FilePath = InsertResult.first->second;
+  if (!InsertResult.second)
----------------
Why is this a reference?


================
Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:552-553
+    }
+    if (!llvm::sys::fs::is_directory(MultipassDir))
+      llvm::sys::fs::create_directory(MultipassDir);
+  }
----------------
This looks like a TOCTOU issue; `create_directory()` takes an argument for whether you should ignore existing or not, that should suffice here, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447