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