[PATCH] D23257: Fix clang-tidy crash when a single fix is applied on multiple files.

[Alexander Kornienko via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

A bunch of nits. Otherwise looks good.

Thank you for the fix!


================
Comment at: clang-tidy/ClangTidy.cpp:519
@@ -513,2 +518,3 @@
   tooling::TranslationUnitReplacements TUR;
   for (const ClangTidyError &Error : Errors)
+    for (const auto &FileAndFixes : Error.Fix)
----------------
Please add braces for the outer loop.

================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:81
@@ -82,1 +80,3 @@
+      tooling::Replacement Replacement(SM, Range, FixIt.CodeToInsert);
+      auto Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
       // FIXME: better error handling.
----------------
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 ;)

================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:498
@@ -497,3 +497,3 @@
     int Size = 0;
-    for (const auto &Replace : Error.Fix)
-      Size += Replace.getLength();
+    for (const auto &FileAndReplaces : Error.Fix)
+      for (const auto &Replace : FileAndReplaces.second)
----------------
Please add braces around the outer loop.

================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:506
@@ -504,15 +505,3 @@
   std::map<std::string, std::vector<Event>> 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 (unsigned I = 0; I < Errors.size(); ++I)
+    for (const auto &FileAndReplace : Errors[I].Fix)
----------------
Please add braces for both outer loops.

================
Comment at: unittests/clang-tidy/ClangTidyTest.h:122
@@ -121,7 +121,3 @@
   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 auto &FileAndFixes : Error.Fix)
+      for (const auto &Fix : FileAndFixes.second) {
----------------
Please add braces here as well.


https://reviews.llvm.org/D23257