[clang-tools-extra] r345961 - [clang-tidy] Get ClangTidyContext out of the business of storing diagnostics. NFC
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 2 03:01:59 PDT 2018
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.
More information about the cfe-commits
mailing list