[clangd] Clean formatting modernize-use-override (llvm#81435)

When applying the recommended fix for the "modernize-use-override" clang-tidy diagnostic there was a stray whitespace. This PR fixes that. Resolves clangd/clangd#1704
nikic · Feb 14, 2024 · 5992b32 · 5992b32
1 parent 1301bc4
commit 5992b32
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 3 deletions.
diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "UseOverrideCheck.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -228,9 +229,14 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   if (HasVirtual) {
     for (Token Tok : Tokens) {
       if (Tok.is(tok::kw_virtual)) {
-        Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-            Tok.getLocation(), Tok.getLocation()));
-        break;
+        std::optional<Token> NextToken =
+            utils::lexer::findNextTokenIncludingComments(
+                Tok.getEndLoc(), Sources, getLangOpts());
+        if (NextToken.has_value()) {
+          Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+              Tok.getLocation(), NextToken->getLocation()));
+          break;
+        }
       }
     }
   }

diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -898,6 +898,46 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiags) {
                 withFix(equalToFix(ExpectedDFix))))));
 }
 
+TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) {
+  Annotations Main(R"cpp(
+    class Interface {
+    public:
+      virtual void Reset1() = 0;
+      virtual void Reset2() = 0;
+    };
+    class A : public Interface {
+      // This will be marked by clangd to use override instead of virtual
+      $virtual1[[virtual    ]]void $Reset1[[Reset1]]()$override1[[]];
+      $virtual2[[virtual      ]]/**/void $Reset2[[Reset2]]()$override2[[]];
+    };
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyProvider =
+      addTidyChecks("cppcoreguidelines-explicit-virtual-functions,");
+  clangd::Fix const ExpectedFix1{
+      "prefer using 'override' or (rarely) 'final' "
+      "instead of 'virtual'",
+      {TextEdit{Main.range("override1"), " override"},
+       TextEdit{Main.range("virtual1"), ""}}};
+  clangd::Fix const ExpectedFix2{
+      "prefer using 'override' or (rarely) 'final' "
+      "instead of 'virtual'",
+      {TextEdit{Main.range("override2"), " override"},
+       TextEdit{Main.range("virtual2"), ""}}};
+  // Note that in the Fix we expect the "virtual" keyword and the following
+  // whitespace to be deleted
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              ifTidyChecks(UnorderedElementsAre(
+                  AllOf(Diag(Main.range("Reset1"),
+                             "prefer using 'override' or (rarely) 'final' "
+                             "instead of 'virtual'"),
+                        withFix(equalToFix(ExpectedFix1))),
+                  AllOf(Diag(Main.range("Reset2"),
+                             "prefer using 'override' or (rarely) 'final' "
+                             "instead of 'virtual'"),
+                        withFix(equalToFix(ExpectedFix2))))));
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -164,6 +164,10 @@ Changes in existing checks
   `AllowStringArrays` option, enabling the exclusion of array types with deduced
   length initialized from string literals.
 
+- Improved :doc:`modernize-use-override
+  <clang-tidy/checks/modernize/use-override>` check to also remove any trailing
+  whitespace when deleting the ``virtual`` keyword.
+
 - Improved :doc:`readability-redundant-inline-specifier
   <clang-tidy/checks/readability/redundant-inline-specifier>` check to properly
   emit warnings for static data member with an in-class initializer.

diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp
@@ -27,6 +27,7 @@ struct Base {
   virtual void f() = 0;
   virtual void f2() const = 0;
   virtual void g() = 0;
+  virtual void g2() = 0;
 
   virtual void j() const;
   virtual MustUseResultObject k();
@@ -126,6 +127,10 @@ struct SimpleCases : public Base {
   virtual void t() throw();
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
   // CHECK-FIXES: {{^}}  void t() throw() override;
+
+  virtual       /*      */ void g2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
+  // CHECK-FIXES: {{^}}  /*      */ void g2() override;
 };
 
 // CHECK-MESSAGES-NOT: warning: