Fix clang-tidy crash when a single fix is applied on multiple files.

[ioeric]

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.

[hokein]

clang-tidy/ClangTidyDiagnosticConsumer.h

66 ↗

(On Diff #67137)

Use llvm::StringMap here?

[alexfh]

clang-tidy/ClangTidy.cpp

129 ↗

(On Diff #67137)

s/Fixes/Replacements/

clang-tidy/ClangTidyDiagnosticConsumer.cpp

80 ↗

(On Diff #67137)

nit: 1. replace is a verb; 2. variable names should start with a capital.

Replacement would be better.

test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp

1 ↗

(On Diff #67137)

I think, cat x > y is preferred over cp x y in lit tests as a more platform-agnostic way of copying files.

[ioeric]

• Addressed reviewers' comments.

[hokein]

test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h

1 ↗

(On Diff #67164)

Usually test should not use STL headers as it relies on the system headers.

You don't have to use std::string to reproduce the crash, IMO.

[alexfh]

test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h

1 ↗

(On Diff #67164)

Yep, just a class with a user-defined move constructor should be fine.

// .h file:
struct S {
  S(S&&);
  S(const S&);
};
struct Foo {
  Foo(const S &s);
  S s;
};

// .cpp file:
#include "a.h"
Foo::Foo(const S &s) : s(s) {}

[ioeric]

• Update test to not use <string>

[alexfh]

A bunch of nits. Otherwise looks good.

Thank you for the fix!

clang-tidy/ClangTidy.cpp

519 ↗

(On Diff #67193)

Please add braces for the outer loop.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

81 ↗

(On Diff #67193)

I was kind of intrigued by what auto stands for here. I think, llvm::Error is a less mysterious way of storing an error here ;)

498 ↗

(On Diff #67193)

Please add braces around the outer loop.

506 ↗

(On Diff #67193)

Please add braces for both outer loops.

unittests/clang-tidy/ClangTidyTest.h

122 ↗

(On Diff #67193)

Please add braces here as well.

[Prazek]

I remember that it was pissing me off when I used clang-tidy first time. Thanks for fixing that!

[ioeric]

• Nits fixed

clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp

SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
       std::string Name = Error.CheckName;
       if (Error.IsWarningAsError) {
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;
         WarningsAsErrors++;
       }
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
                   << Message.Message << Name;
-      for (const tooling::Replacement &Fix : Error.Fix) {
+      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 (Fix.isApplicable()) {
-          SmallString<128> FixAbsoluteFilePath = Fix.getFilePath();
+          if (Replacement.isApplicable()) {
+            SmallString<128> FixAbsoluteFilePath = Replacement.getFilePath();
             Files.makeAbsolutePath(FixAbsoluteFilePath);
-          FixLoc = getLocation(FixAbsoluteFilePath, Fix.getOffset());
-          SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Fix.getLength());
+            FixLoc = getLocation(FixAbsoluteFilePath, Replacement.getOffset());
+            SourceLocation FixEndLoc =
+                FixLoc.getLocWithOffset(Replacement.getLength());
             Range = SourceRange(FixLoc, FixEndLoc);
-          Diag << FixItHint::CreateReplacement(Range, Fix.getReplacementText());
+            Diag << FixItHint::CreateReplacement(
+                Range, Replacement.getReplacementText());
           }
 
           ++TotalFixes;
           if (ApplyFixes) {
-          bool Success = Fix.isApplicable() && Fix.apply(Rewrite);
+            bool Success =
+                Replacement.isApplicable() && Replacement.apply(Rewrite);
             if (Success)
               ++AppliedFixes;
             FixLocations.push_back(std::make_pair(FixLoc, Success));
           }
         }
       }
+    }
     for (auto Fix : FixLocations) {
       Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
                                          : diag::note_fixit_failed);
     }
     for (const ClangTidyMessage &Note : Error.Notes)
       reportNote(Note);
   }
 
void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
   }
   Reporter.Finish();
   WarningsAsErrorsCount += Reporter.getWarningsAsErrorsCount();
 }
 
 void exportReplacements(const std::vector<ClangTidyError> &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;
 }
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h

enum Level {
     Error = DiagnosticsEngine::Error
   };
 
   ClangTidyError(StringRef CheckName, Level DiagLevel, bool IsWarningAsError,
                  StringRef BuildDirectory);
 
   std::string CheckName;
   ClangTidyMessage Message;
-  tooling::Replacements Fix;
+  // Fixes grouped by file path.
+  llvm::StringMap<tooling::Replacements> Fix;
   SmallVector<ClangTidyMessage, 1> Notes;
 
   // A build directory of the diagnostic source file.
   //
   // It's an absolute path which is `directory` field of the source file in
   // compilation database. If users don't specify the compilation database
   // directory, it is the current directory where clang-tidy runs.
   //

clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp

void emitCodeContext(SourceLocation Loc, DiagnosticsEngine::Level Level,
     assert(Loc.isValid());
     for (const auto &FixIt : Hints) {
       CharSourceRange Range = FixIt.RemoveRange;
       assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
              "Invalid range in the fix-it hint.");
       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! "
                     << llvm::toString(std::move(Err)) << "\n";
       }
       assert(!Err && "Fix conflicts with existing fix!");
     }
   }
struct Event {
     // The events will be sorted based on this field.
     std::tuple<unsigned, EventType, int, int, unsigned> Priority;
   };
 
   // Compute error sizes.
   std::vector<int> Sizes;
   for (const auto &Error : Errors) {
     int Size = 0;
-    for (const auto &Replace : Error.Fix)
+    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<std::string, std::vector<Event>> FileEvents;
   for (unsigned I = 0; I < Errors.size(); ++I) {
-    for (const auto &Replace : Errors[I].Fix) {
+    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]));
       }
     }
+  }
 
   std::vector<bool> Apply(Errors.size(), true);
   for (auto &FileAndEvents : FileEvents) {
     std::vector<Event> &Events = FileAndEvents.second;
     // Sweep.
     std::sort(Events.begin(), Events.end());
     int OpenIntervals = 0;
     for (const auto &Event : Events) {

clang-tools-extra/trunk/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h

+struct S {
+  S(S&&);
+  S(const S&);
+};
+struct Foo {
+  Foo(const S &s);
+  S s;
+};

clang-tools-extra/trunk/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp

+// 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 <utility>
+// CHECK-FIXES: Foo::Foo(S s) : s(std::move(s)) {}

clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h

if (!Invocation.run()) {
     for (const auto &Error : Context.getErrors()) {
       ErrorText += Error.Message.Message + "\n";
     }
     llvm::report_fatal_error(ErrorText);
   }
 
   DiagConsumer.finish();
   tooling::Replacements Fixes;
-  for (const ClangTidyError &Error : Context.getErrors())
-    for (const auto &Fix : Error.Fix) {
+  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);
   if (!Result) {
     // FIXME: propogate the error.
     llvm::consumeError(Result.takeError());
     return "";
   }