From 7e5445267f8adc66db66ca09c2804cf8f6ebb960 Mon Sep 17 00:00:00 2001 From: Eric Liu Date: Tue, 9 Aug 2016 07:54:49 +0000 Subject: [PATCH] Fix clang-tidy crash when a single fix is applied on multiple files. Summary: tooling::Replacements only holds replacements for a single file, so this patch makes Fix a map from file paths to tooling::Replacements so that it can be applied on multiple files. Reviewers: hokein, alexfh Subscribers: Prazek, cfe-commits Differential Revision: https://reviews.llvm.org/D23257 llvm-svn: 278101 --- clang-tools-extra/clang-tidy/ClangTidy.cpp | 56 ++++++++++++---------- .../clang-tidy/ClangTidyDiagnosticConsumer.cpp | 34 +++++++------ .../clang-tidy/ClangTidyDiagnosticConsumer.h | 3 +- .../modernize-pass-by-value/header-with-fix.h | 8 ++++ .../modernize-pass-by-value-multi-fixes.cpp | 12 +++++ .../unittests/clang-tidy/ClangTidyTest.h | 17 ++++--- 6 files changed, 83 insertions(+), 47 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h create mode 100644 clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 562e1e8..f2ad5b2 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -126,27 +126,32 @@ public: } auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]")) << Message.Message << Name; - for (const tooling::Replacement &Fix : Error.Fix) { - // Retrieve the source range for applicable fixes. Macro definitions - // on the command line have locations in a virtual buffer and don't - // have valid file paths and are therefore not applicable. - SourceRange Range; - SourceLocation FixLoc; - if (Fix.isApplicable()) { - SmallString<128> FixAbsoluteFilePath = Fix.getFilePath(); - Files.makeAbsolutePath(FixAbsoluteFilePath); - FixLoc = getLocation(FixAbsoluteFilePath, Fix.getOffset()); - SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Fix.getLength()); - Range = SourceRange(FixLoc, FixEndLoc); - Diag << FixItHint::CreateReplacement(Range, Fix.getReplacementText()); - } - - ++TotalFixes; - if (ApplyFixes) { - bool Success = Fix.isApplicable() && Fix.apply(Rewrite); - if (Success) - ++AppliedFixes; - FixLocations.push_back(std::make_pair(FixLoc, Success)); + for (const auto &FileAndReplacements : Error.Fix) { + for (const auto &Replacement : FileAndReplacements.second) { + // Retrieve the source range for applicable fixes. Macro definitions + // on the command line have locations in a virtual buffer and don't + // have valid file paths and are therefore not applicable. + SourceRange Range; + SourceLocation FixLoc; + if (Replacement.isApplicable()) { + SmallString<128> FixAbsoluteFilePath = Replacement.getFilePath(); + Files.makeAbsolutePath(FixAbsoluteFilePath); + FixLoc = getLocation(FixAbsoluteFilePath, Replacement.getOffset()); + SourceLocation FixEndLoc = + FixLoc.getLocWithOffset(Replacement.getLength()); + Range = SourceRange(FixLoc, FixEndLoc); + Diag << FixItHint::CreateReplacement( + Range, Replacement.getReplacementText()); + } + + ++TotalFixes; + if (ApplyFixes) { + bool Success = + Replacement.isApplicable() && Replacement.apply(Rewrite); + if (Success) + ++AppliedFixes; + FixLocations.push_back(std::make_pair(FixLoc, Success)); + } } } } @@ -511,9 +516,12 @@ void handleErrors(const std::vector &Errors, bool Fix, void exportReplacements(const std::vector &Errors, raw_ostream &OS) { tooling::TranslationUnitReplacements TUR; - for (const ClangTidyError &Error : Errors) - TUR.Replacements.insert(TUR.Replacements.end(), Error.Fix.begin(), - Error.Fix.end()); + for (const ClangTidyError &Error : Errors) { + for (const auto &FileAndFixes : Error.Fix) + TUR.Replacements.insert(TUR.Replacements.end(), + FileAndFixes.second.begin(), + FileAndFixes.second.end()); + } yaml::Output YAML(OS); YAML << TUR; diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 940b063..8dbae40 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -77,8 +77,8 @@ protected: assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() && "Only file locations supported in fix-it hints."); - auto Err = - Error.Fix.add(tooling::Replacement(SM, Range, FixIt.CodeToInsert)); + tooling::Replacement Replacement(SM, Range, FixIt.CodeToInsert); + llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement); // FIXME: better error handling. if (Err) { llvm::errs() << "Fix conflicts with existing fix! " @@ -495,25 +495,29 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors( std::vector Sizes; for (const auto &Error : Errors) { int Size = 0; - for (const auto &Replace : Error.Fix) - Size += Replace.getLength(); + for (const auto &FileAndReplaces : Error.Fix) { + for (const auto &Replace : FileAndReplaces.second) + Size += Replace.getLength(); + } Sizes.push_back(Size); } // Build events from error intervals. std::map> FileEvents; for (unsigned I = 0; I < Errors.size(); ++I) { - for (const auto &Replace : Errors[I].Fix) { - unsigned Begin = Replace.getOffset(); - unsigned End = Begin + Replace.getLength(); - const std::string &FilePath = Replace.getFilePath(); - // FIXME: Handle empty intervals, such as those from insertions. - if (Begin == End) - continue; - FileEvents[FilePath].push_back( - Event(Begin, End, Event::ET_Begin, I, Sizes[I])); - FileEvents[FilePath].push_back( - Event(Begin, End, Event::ET_End, I, Sizes[I])); + for (const auto &FileAndReplace : Errors[I].Fix) { + for (const auto &Replace : FileAndReplace.second) { + unsigned Begin = Replace.getOffset(); + unsigned End = Begin + Replace.getLength(); + const std::string &FilePath = Replace.getFilePath(); + // FIXME: Handle empty intervals, such as those from insertions. + if (Begin == End) + continue; + FileEvents[FilePath].push_back( + Event(Begin, End, Event::ET_Begin, I, Sizes[I])); + FileEvents[FilePath].push_back( + Event(Begin, End, Event::ET_End, I, Sizes[I])); + } } } diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index d7e4d2b..9f83aac 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -62,7 +62,8 @@ struct ClangTidyError { std::string CheckName; ClangTidyMessage Message; - tooling::Replacements Fix; + // Fixes grouped by file path. + llvm::StringMap Fix; SmallVector Notes; // A build directory of the diagnostic source file. diff --git a/clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h b/clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h new file mode 100644 index 0000000..62609e2 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h @@ -0,0 +1,8 @@ +struct S { + S(S&&); + S(const S&); +}; +struct Foo { + Foo(const S &s); + S s; +}; diff --git a/clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp b/clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp new file mode 100644 index 0000000..7980c30 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp @@ -0,0 +1,12 @@ +// RUN: cat %S/Inputs/modernize-pass-by-value/header-with-fix.h > %T/pass-by-value-header-with-fix.h +// RUN: sed -e 's#//.*$##' %s > %t.cpp +// RUN: clang-tidy %t.cpp -checks='-*,modernize-pass-by-value' -header-filter='.*' -fix -- -std=c++11 -I %T | FileCheck %s -check-prefix=CHECK-MESSAGES -implicit-check-not="{{warning|error}}:" +// RUN: FileCheck -input-file=%t.cpp %s -check-prefix=CHECK-FIXES +// RUN: FileCheck -input-file=%T/pass-by-value-header-with-fix.h %s -check-prefix=CHECK-HEADER-FIXES + +#include "pass-by-value-header-with-fix.h" +// CHECK-HEADER-FIXES: Foo(S s); +Foo::Foo(const S &s) : s(s) {} +// CHECK-MESSAGES: :9:10: warning: pass by value and use std::move [modernize-pass-by-value] +// CHECK-FIXES: #include +// CHECK-FIXES: Foo::Foo(S s) : s(std::move(s)) {} diff --git a/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h b/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h index 0e278f8..6dfce90 100644 --- a/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h +++ b/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h @@ -118,15 +118,18 @@ runCheckOnCode(StringRef Code, std::vector *Errors = nullptr, DiagConsumer.finish(); tooling::Replacements Fixes; - for (const ClangTidyError &Error : Context.getErrors()) - for (const auto &Fix : Error.Fix) { - auto Err = Fixes.add(Fix); - // FIXME: better error handling. Keep the behavior for now. - if (Err) { - llvm::errs() << llvm::toString(std::move(Err)) << "\n"; - return ""; + for (const ClangTidyError &Error : Context.getErrors()) { + for (const auto &FileAndFixes : Error.Fix) { + for (const auto &Fix : FileAndFixes.second) { + auto Err = Fixes.add(Fix); + // FIXME: better error handling. Keep the behavior for now. + if (Err) { + llvm::errs() << llvm::toString(std::move(Err)) << "\n"; + return ""; + } } } + } if (Errors) *Errors = Context.getErrors(); auto Result = tooling::applyAllReplacements(Code, Fixes); -- 2.7.4