Page MenuHomePhabricator

[clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros
AcceptedPublic

Authored by steakhal on Wed, Nov 10, 3:54 AM.

Details

Summary

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.

Diff Detail

Event Timeline

steakhal created this revision.Wed, Nov 10, 3:54 AM
steakhal requested review of this revision.Wed, Nov 10, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 10, 3:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
whisperity accepted this revision.Mon, Nov 15, 12:56 AM

The fix is concise, thank you for observing and untangling the crash!

As the check was originally introduced in the ongoing release, we can go without a release notes entry.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
288

It is strange that there is no fix-it here even though the keyword appears as a single token ...[1]

288

Maybe a FIXME could be added for this case, just to register that we indeed realised something is strange here, but I'm not convinced neither for nor against. The purpose of the patch is to get rid of the crash, after all.

299

[1]... whereas here the check generates the fixit which removes the entire macro substitution!

This revision is now accepted and ready to land.Mon, Nov 15, 12:56 AM
steakhal updated this revision to Diff 387189.Mon, Nov 15, 2:59 AM
steakhal marked 2 inline comments as done.

Add fixme comment for:

class FooBar2 {
protected:
  virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this.
};

ping

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp
288

Definitely deserves a FIXME. Yes.

whisperity accepted this revision.Fri, Nov 26, 12:12 AM

I'm not sure if it is a good idea to merge things on a Friday, but I am comfortable with this, let's get the check crash less.

I'm not sure if it is a good idea to merge things on a Friday, but I am comfortable with this, let's get the check crash less.

I've run this on a few projects and it did not introduce new crashes. I plan to commit this on Monday.