[clang-tidy] Address post-commit comments

hokein · hokein · commit 8bbbd31cdd4a · 2019-04-18T14:18:14.000Z
Summary: Also add a test to verify clang-tidy only apply the first alternative fix. Reviewers: alexfh Subscribers: xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D60857 llvm-svn: 358666
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -133,41 +133,40 @@ class ErrorReporter {
           for (const auto &Repl : FileAndReplacements.second) {
             ++TotalFixes;
             bool CanBeApplied = false;
-            if (Repl.isApplicable()) {
-              SourceLocation FixLoc;
-              SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
-              Files.makeAbsolutePath(FixAbsoluteFilePath);
-              tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
-                                     Repl.getLength(),
-                                     Repl.getReplacementText());
-              Replacements &Replacements = FileReplacements[R.getFilePath()];
-              llvm::Error Err = Replacements.add(R);
-              if (Err) {
-                // FIXME: Implement better conflict handling.
-                llvm::errs() << "Trying to resolve conflict: "
-                             << llvm::toString(std::move(Err)) << "\n";
-                unsigned NewOffset =
-                    Replacements.getShiftedCodePosition(R.getOffset());
-                unsigned NewLength = Replacements.getShiftedCodePosition(
-                                         R.getOffset() + R.getLength()) -
-                                     NewOffset;
-                if (NewLength == R.getLength()) {
-                  R = Replacement(R.getFilePath(), NewOffset, NewLength,
-                                  R.getReplacementText());
-                  Replacements = Replacements.merge(tooling::Replacements(R));
-                  CanBeApplied = true;
-                  ++AppliedFixes;
-                } else {
-                  llvm::errs()
-                      << "Can't resolve conflict, skipping the replacement.\n";
-                }
-              } else {
+            if (!Repl.isApplicable())
+              continue;
+            SourceLocation FixLoc;
+            SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
+            Files.makeAbsolutePath(FixAbsoluteFilePath);
+            tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
+                                   Repl.getLength(), Repl.getReplacementText());
+            Replacements &Replacements = FileReplacements[R.getFilePath()];
+            llvm::Error Err = Replacements.add(R);
+            if (Err) {
+              // FIXME: Implement better conflict handling.
+              llvm::errs() << "Trying to resolve conflict: "
+                           << llvm::toString(std::move(Err)) << "\n";
+              unsigned NewOffset =
+                  Replacements.getShiftedCodePosition(R.getOffset());
+              unsigned NewLength = Replacements.getShiftedCodePosition(
+                                       R.getOffset() + R.getLength()) -
+                                   NewOffset;
+              if (NewLength == R.getLength()) {
+                R = Replacement(R.getFilePath(), NewOffset, NewLength,
+                                R.getReplacementText());
+                Replacements = Replacements.merge(tooling::Replacements(R));
                 CanBeApplied = true;
                 ++AppliedFixes;
+              } else {
+                llvm::errs()
+                    << "Can't resolve conflict, skipping the replacement.\n";
               }
-              FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
-              FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
+            } else {
+              CanBeApplied = true;
+              ++AppliedFixes;
             }
+            FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
+            FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
           }
         }
       }
diff --git a/clang-tools-extra/test/clang-tidy/alternative-fixes.cpp b/clang-tools-extra/test/clang-tidy/alternative-fixes.cpp
@@ -0,0 +1,9 @@
+// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t
+void foo(int a) {
+  if (a = 1) {
+  // CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
+  // CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
+  // CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
+  // CHECK-FIXES: if ((a = 1)) {
+  }
+}
diff --git a/clang/include/clang/Tooling/Core/Diagnostic.h b/clang/include/clang/Tooling/Core/Diagnostic.h
@@ -93,8 +93,8 @@ struct TranslationUnitDiagnostics {
   std::vector<Diagnostic> Diagnostics;
 };
 
-// Get the first fix to apply for this diagnostic.
-// Return nullptr if no fixes attached to the diagnostic.
+/// Get the first fix to apply for this diagnostic.
+/// \returns nullptr if no fixes are attached to the diagnostic.
 const llvm::StringMap<Replacements> *selectFirstFix(const Diagnostic& D);
 
 } // end namespace tooling
