steakhal created this revision.
steakhal added reviewers: aaron.ballman, njames93, alexfh, mgartmann,
whisperity.
Herald added subscribers: carlosgalvezp, martong, shchenz, rnkovacs, kbarton,
xazax.hun, nemanjai.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
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.
https://reviews.llvm.org/D113558
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
Index:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
---
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -270,3 +270,53 @@
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()); // no-fixit
+};
+
+// 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
Index:
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
---
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -46,9 +46,12 @@
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 @@
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,
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
@@ -270,3 +270,53 @@
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()); // no-fixit
+};
+
+// 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
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
@@ -46,9 +46,12 @@
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 @@
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,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
