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