Author: sammccall Date: Fri Nov 2 03:01:59 2018 New Revision: 345961 URL: http://llvm.org/viewvc/llvm-project?rev=345961&view=rev Log: [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC
Summary: Currently ClangTidyContext::diag() sends the diagnostics to a DiagnosticsEngine, which probably delegates to a ClangTidyDiagnosticsConsumer, which is supposed to go back and populate ClangTidyContext::Errors. After this patch, the diagnostics are stored in the ClangTidyDiagnosticsConsumer itself and can be retrieved from there. Why? - the round-trip from context -> engine -> consumer -> context is confusing and makes it harder to establish layering between these things. - context does too many things, and makes it hard to use clang-tidy as a library - everyone who actually wants the diagnostics has access to the ClangTidyDiagnosticsConsumer The most natural implementation (ClangTidyDiagnosticsConsumer::take() finalizes diagnostics) causes a test failure: clang-tidy-run-with-database.cpp asserts that clang-tidy exits successfully when trying to process a file that doesn't exist. In clang-tidy today, this happens because finish() is never called, so the diagnostic is never flushed. This looks like a bug to me. For now, this patch carefully preserves that behavior, but I'll ping the authors to see whether it's deliberate and worth preserving. Reviewers: hokein Subscribers: xazax.hun, cfe-commits, alexfh Differential Revision: https://reviews.llvm.org/D53953 Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp clang-tools-extra/trunk/clang-tidy/ClangTidy.h clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp Fri Nov 2 03:01:59 2018 @@ -500,11 +500,12 @@ getCheckOptions(const ClangTidyOptions & return Factory.getCheckOptions(); } -void runClangTidy(clang::tidy::ClangTidyContext &Context, - const CompilationDatabase &Compilations, - ArrayRef<std::string> InputFiles, - llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS, - bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) { +std::vector<ClangTidyError> +runClangTidy(clang::tidy::ClangTidyContext &Context, + const CompilationDatabase &Compilations, + ArrayRef<std::string> InputFiles, + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS, + bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) { ClangTool Tool(Compilations, InputFiles, std::make_shared<PCHContainerOperations>(), BaseFS); @@ -586,9 +587,11 @@ void runClangTidy(clang::tidy::ClangTidy ActionFactory Factory(Context); Tool.run(&Factory); + return DiagConsumer.take(); } -void handleErrors(ClangTidyContext &Context, bool Fix, +void handleErrors(llvm::ArrayRef<ClangTidyError> Errors, + ClangTidyContext &Context, bool Fix, unsigned &WarningsAsErrorsCount, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) { ErrorReporter Reporter(Context, Fix, BaseFS); @@ -598,7 +601,7 @@ void handleErrors(ClangTidyContext &Cont if (!InitialWorkingDir) llvm::report_fatal_error("Cannot get current working path."); - for (const ClangTidyError &Error : Context.getErrors()) { + for (const ClangTidyError &Error : Errors) { if (!Error.BuildDirectory.empty()) { // By default, the working directory of file system is the current // clang-tidy running directory. Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.h?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/ClangTidy.h (original) +++ clang-tools-extra/trunk/clang-tidy/ClangTidy.h Fri Nov 2 03:01:59 2018 @@ -230,12 +230,13 @@ getCheckOptions(const ClangTidyOptions & /// \param StoreCheckProfile If provided, and EnableCheckProfile is true, /// the profile will not be output to stderr, but will instead be stored /// as a JSON file in the specified directory. -void runClangTidy(clang::tidy::ClangTidyContext &Context, - const tooling::CompilationDatabase &Compilations, - ArrayRef<std::string> InputFiles, - llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS, - bool EnableCheckProfile = false, - llvm::StringRef StoreCheckProfile = StringRef()); +std::vector<ClangTidyError> +runClangTidy(clang::tidy::ClangTidyContext &Context, + const tooling::CompilationDatabase &Compilations, + ArrayRef<std::string> InputFiles, + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS, + bool EnableCheckProfile = false, + llvm::StringRef StoreCheckProfile = StringRef()); // FIXME: This interface will need to be significantly extended to be useful. // FIXME: Implement confidence levels for displaying/fixing errors. @@ -243,7 +244,8 @@ void runClangTidy(clang::tidy::ClangTidy /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p /// Errors containing fixes are automatically applied and reformatted. If no /// clang-format configuration file is found, the given \P FormatStyle is used. -void handleErrors(ClangTidyContext &Context, bool Fix, +void handleErrors(llvm::ArrayRef<ClangTidyError> Errors, + ClangTidyContext &Context, bool Fix, unsigned &WarningsAsErrorsCount, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS); Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Fri Nov 2 03:01:59 2018 @@ -525,8 +525,7 @@ llvm::Regex *ClangTidyDiagnosticConsumer return HeaderFilter.get(); } -void ClangTidyDiagnosticConsumer::removeIncompatibleErrors( - SmallVectorImpl<ClangTidyError> &Errors) const { +void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() { // Each error is modelled as the set of intervals in which it applies // replacements. To detect overlapping replacements, we use a sweep line // algorithm over these sets of intervals. @@ -664,17 +663,13 @@ struct EqualClangTidyError { }; } // end anonymous namespace -// Flushes the internal diagnostics buffer to the ClangTidyContext. -void ClangTidyDiagnosticConsumer::finish() { +std::vector<ClangTidyError> ClangTidyDiagnosticConsumer::take() { finalizeLastError(); std::sort(Errors.begin(), Errors.end(), LessClangTidyError()); Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()), Errors.end()); - if (RemoveIncompatibleErrors) - removeIncompatibleErrors(Errors); - - std::move(Errors.begin(), Errors.end(), std::back_inserter(Context.Errors)); - Errors.clear(); + removeIncompatibleErrors(); + return std::move(Errors); } Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h (original) +++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h Fri Nov 2 03:01:59 2018 @@ -160,12 +160,6 @@ public: /// counters. const ClangTidyStats &getStats() const { return Stats; } - /// \brief Returns all collected errors. - ArrayRef<ClangTidyError> getErrors() const { return Errors; } - - /// \brief Clears collected errors. - void clearErrors() { Errors.clear(); } - /// \brief Control profile collection in clang-tidy. void setEnableProfiling(bool Profile); bool getEnableProfiling() const { return Profile; } @@ -196,7 +190,6 @@ private: friend class ClangTidyDiagnosticConsumer; friend class ClangTidyPluginAction; - std::vector<ClangTidyError> Errors; DiagnosticsEngine *DiagEngine; std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider; @@ -236,13 +229,12 @@ public: void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) override; - /// \brief Flushes the internal diagnostics buffer to the ClangTidyContext. - void finish() override; + // Retrieve the diagnostics that were captured. + std::vector<ClangTidyError> take(); private: void finalizeLastError(); - - void removeIncompatibleErrors(SmallVectorImpl<ClangTidyError> &Errors) const; + void removeIncompatibleErrors(); /// \brief Returns the \c HeaderFilter constructed for the options set in the /// context. @@ -256,7 +248,7 @@ private: ClangTidyContext &Context; bool RemoveIncompatibleErrors; std::unique_ptr<DiagnosticsEngine> Diags; - SmallVector<ClangTidyError, 8> Errors; + std::vector<ClangTidyError> Errors; std::unique_ptr<llvm::Regex> HeaderFilter; bool LastErrorRelatesToUserCode; bool LastErrorPassesLineFilter; Modified: clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp Fri Nov 2 03:01:59 2018 @@ -422,9 +422,9 @@ static int clangTidyMain(int argc, const ClangTidyContext Context(std::move(OwningOptionsProvider), AllowEnablingAnalyzerAlphaCheckers); - runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS, - EnableCheckProfile, ProfilePrefix); - ArrayRef<ClangTidyError> Errors = Context.getErrors(); + std::vector<ClangTidyError> Errors = + runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS, + EnableCheckProfile, ProfilePrefix); bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) { return E.DiagLevel == ClangTidyError::Error; }) != Errors.end(); @@ -434,7 +434,7 @@ static int clangTidyMain(int argc, const unsigned WErrorCount = 0; // -fix-errors implies -fix. - handleErrors(Context, (FixErrors || Fix) && !DisableFixes, WErrorCount, + handleErrors(Errors, Context, (FixErrors || Fix) && !DisableFixes, WErrorCount, BaseFS); if (!ExportFixes.empty() && !Errors.empty()) { Modified: clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp Fri Nov 2 03:01:59 2018 @@ -8,7 +8,13 @@ // RUN: echo 'int *HP = 0;' > %T/compilation-database-test/include/header.h // RUN: echo '#include "header.h"' > %T/compilation-database-test/b/d.cpp // RUN: sed 's|test_dir|%/T/compilation-database-test|g' %S/Inputs/compilation-database/template.json > %T/compile_commands.json -// RUN: clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/b/not-exist -header-filter=.* + +// Regression test: shouldn't crash. +// RUN: not clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/b/not-exist -header-filter=.* 2>&1 | FileCheck %s -check-prefix=CHECK-NOT-EXIST +// CHECK-NOT-EXIST: Error while processing {{.*}}/not-exist. +// CHECK-NOT-EXIST: unable to handle compilation +// CHECK-NOT-EXIST: Found compiler error + // RUN: clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/a/a.cpp %T/compilation-database-test/a/b.cpp %T/compilation-database-test/b/b.cpp %T/compilation-database-test/b/c.cpp %T/compilation-database-test/b/d.cpp -header-filter=.* -fix // RUN: FileCheck -input-file=%T/compilation-database-test/a/a.cpp %s -check-prefix=CHECK-FIX1 // RUN: FileCheck -input-file=%T/compilation-database-test/a/b.cpp %s -check-prefix=CHECK-FIX2 Modified: clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h?rev=345961&r1=345960&r2=345961&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h (original) +++ clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h Fri Nov 2 03:01:59 2018 @@ -118,15 +118,15 @@ runCheckOnCode(StringRef Code, std::vect Invocation.setDiagnosticConsumer(&DiagConsumer); if (!Invocation.run()) { std::string ErrorText; - for (const auto &Error : Context.getErrors()) { + for (const auto &Error : DiagConsumer.take()) { ErrorText += Error.Message.Message + "\n"; } llvm::report_fatal_error(ErrorText); } - DiagConsumer.finish(); tooling::Replacements Fixes; - for (const ClangTidyError &Error : Context.getErrors()) { + std::vector<ClangTidyError> Diags = DiagConsumer.take(); + for (const ClangTidyError &Error : Diags) { for (const auto &FileAndFixes : Error.Fix) { for (const auto &Fix : FileAndFixes.second) { auto Err = Fixes.add(Fix); @@ -139,7 +139,7 @@ runCheckOnCode(StringRef Code, std::vect } } if (Errors) - *Errors = Context.getErrors(); + *Errors = std::move(Diags); auto Result = tooling::applyAllReplacements(Code, Fixes); if (!Result) { // FIXME: propogate the error. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
