[clang-tools-extra] r345961 - [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC
Tom Weaver via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 2 05:04:15 PDT 2018
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 at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181102/fab6b202/attachment-0001.html>
More information about the cfe-commits
mailing list