[PATCH] D64274: [analyzer] VirtualCallChecker overhaul.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

Thank you, and sorry for dragging you through this! At the very least, we got to learn a lot from it :)



================
Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:93-96
 // This wrapper is used to ensure that only StringRefs originating from the
 // CheckerRegistry are used as check names. We want to make sure all check
 // name strings have a lifetime that keeps them alive at least until the path
 // diagnostics have been processed.
----------------
Yea, so this comment isn't incorrect, but it isn't obvious the reason behind this: `CheckerRegistry` gets destructed almost immediately after loading the checkers to `CheckerManager`, the thing we should emphasize here is that those checker names are actually generated as string literals with the inclusion of `Checkers.inc`, which is why their lifetime is long enough (practically infinite).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:97
 // diagnostics have been processed.
 class CheckName {
   friend class ::clang::ento::CheckerRegistry;
----------------
Grrrr, this has been bugging me for far too long. We should totally rename this to `CheckerName` eventually.


================
Comment at: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp:14-20
 const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
 const char * const LogicError = "Logic error";
 const char * const MemoryRefCount =
   "Memory (Core Foundation/Objective-C/OSObject)";
 const char * const MemoryError = "Memory error";
 const char * const UnixAPI = "Unix API";
+const char * const CXXObjectLifecycle = "C++ object lifecycle";
----------------
`static constexpr StringLiteral`? Why is there a need to have a header and a cpp file for this? Not that it matters much (definitely not in the context of this patch), but compile-time stuff is cool.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64274/new/

https://reviews.llvm.org/D64274