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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
