[clang-tools-extra] 0685e83 - Fix cppcoreguidelines-virtual-base-class-destructor in macros

[Balazs Benics via cfe-commits]

Author: Balazs Benics
Date: 2021-11-29T09:56:43+01:00
New Revision: 0685e83534ef8917f277b394da2927cabff8129f

URL: https://github.com/llvm/llvm-project/commit/0685e83534ef8917f277b394da2927cabff8129f
DIFF: https://github.com/llvm/llvm-project/commit/0685e83534ef8917f277b394da2927cabff8129f.diff

LOG: Fix cppcoreguidelines-virtual-base-class-destructor in macros

The `cppcoreguidelines-virtual-base-class-destructor` checker crashed on
this example:

  #define DECLARE(CLASS) \
  class CLASS {          \
  protected:             \
    virtual ~CLASS();    \
  }
  DECLARE(Foo); // no-crash

The checker will hit the following assertion:

  clang-tidy: llvm/include/llvm/ADT/Optional.h:196: T &llvm::optional_detail::OptionalStorage<clang::Token, true>::getValue() & [T = clang::Token]: Assertion `hasVal' failed."

It turns out, `Lexer::findNextToken()` returned `llvm::None` within the
`getVirtualKeywordRange()` function when the `VirtualEndLoc`
SourceLocation represents a macro expansion.
To prevent this from happening, I decided to propagate the `llvm::None`
further up and only create the removal of `virtual` if the
`getVirtualKeywordRange()` succeeds.

I considered an alternative fix for this issue:
I could have checked the `Destructor.getLocation().isMacroID()` before
doing any Fixit calculation inside the `check()` function.
In contrast to this approach my patch will preserve the diagnostics and
drop the fixits only if it would have crashed.

Reviewed By: whisperity

Differential Revision: https://reviews.llvm.org/D113558

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
    clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
index 0cf7cc9ad6d52..fa5e06c8e7b26 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -46,9 +46,12 @@ void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
       this);
 }
 
-static CharSourceRange
+static Optional<CharSourceRange>
 getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
                        const SourceManager &SM, const LangOptions &LangOpts) {
+  if (Destructor.getLocation().isMacroID())
+    return None;
+
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
   SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
       Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
@@ -190,8 +193,10 @@ void VirtualClassDestructorCheck::check(
       Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual ");
     } else if (Destructor->getAccess() == AccessSpecifier::AS_protected) {
       ProtectedAndVirtual = true;
-      Fix = FixItHint::CreateRemoval(getVirtualKeywordRange(
-          *Destructor, *Result.SourceManager, Result.Context->getLangOpts()));
+      if (const auto MaybeRange =
+              getVirtualKeywordRange(*Destructor, *Result.SourceManager,
+                                     Result.Context->getLangOpts()))
+        Fix = FixItHint::CreateRemoval(*MaybeRange);
     }
   } else {
     Fix = generateUserDeclaredDestructor(*MatchedClassOrStruct,

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
index 51535f89ac43d..c14a2e68def88 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -270,3 +270,53 @@ using DerivedFromTemplateNonVirtualBaseStruct2Typedef = DerivedFromTemplateNonVi
 DerivedFromTemplateNonVirtualBaseStruct2Typedef InstantiationWithPublicNonVirtualBaseStruct2;
 
 } // namespace Bugzilla_51912
+
+namespace macro_tests {
+#define CONCAT(x, y) x##y
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar1 {
+protected:
+  CONCAT(vir, tual) CONCAT(~Foo, Bar1()); // no-fixit
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar2' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar2 {
+protected:
+  virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this.
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'FooBar3' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it protected and non-virtual
+// CHECK-FIXES:      class FooBar3 {
+// CHECK-FIXES-NEXT: protected:
+// CHECK-FIXES-NEXT:   ~FooBar3();
+// CHECK-FIXES-NEXT: };
+class FooBar3 {
+protected:
+  CONCAT(vir, tual) ~FooBar3();
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'FooBar4' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it protected and non-virtual
+// CHECK-FIXES:      class FooBar4 {
+// CHECK-FIXES-NEXT: protected:
+// CHECK-FIXES-NEXT:   ~CONCAT(Foo, Bar4());
+// CHECK-FIXES-NEXT: };
+class FooBar4 {
+protected:
+  CONCAT(vir, tual) ~CONCAT(Foo, Bar4());
+};
+
+// CHECK-MESSAGES: :[[@LINE+3]]:7: warning: destructor of 'FooBar5' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+2]]:7: note: make it protected and non-virtual
+#define XMACRO(COLUMN1, COLUMN2) COLUMN1 COLUMN2
+class FooBar5 {
+protected:
+  XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
+};
+#undef XMACRO
+#undef CONCAT
+} // namespace macro_tests