Fix cppcoreguidelines-virtual-base-class-destructor in macros
authorBalazs Benics <balazs.benics@sigmatechnology.se>
Mon, 29 Nov 2021 08:56:43 +0000 (09:56 +0100)
committerBalazs Benics <balazs.benics@sigmatechnology.se>
Mon, 29 Nov 2021 08:56:43 +0000 (09:56 +0100)
commit0685e83534ef8917f277b394da2927cabff8129f
tree7496c1a7b4d612cb51d2153be859ab6655a67fe4
parenta31f4bdfe8211ecb38741c4fd570baf0d6e16f76
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
clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp