Hiya Sam, are you aware that r345961 caused a test failure on the following bot and build
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/21169 is there any chance you could take a look please? thanks Tom On Fri, 2 Nov 2018 at 10:04, Sam McCall via cfe-commits < cfe-commits@lists.llvm.org> wrote: > 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 >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits