Page MenuHomePhabricator

[analyzer][NFC] Introduce SuperChecker<>, a convenient alternative to Checker<> for storing subcheckers
Needs ReviewPublic

Authored by Szelethus on Sun, Sep 8, 3:08 PM.

Details

Summary

Please don't take a shot for each time I write checker, it'd end bad.

The term "super checker" or "modeling checker", and the term "subchecker" always existed conceptually, but they weren't ever formalized. In the last half a year or so, we referred to them as

  • Super/modeling checker: A checker that models some C++ code, but doesn't (or, as of now, just shouldn't) emit any diagnostics, at least not under its own name.
  • Subcheckers: Checkers that are a part of super/modeling checkers, enabling/disabling them (ideally) only toggles whether a diagnostic is emitted from the checker it is a part of. They don't possess a checker object on their own, and are basically glorified checker options.

While checker dependencies were in similar shoes (existed conceptually but not formalized), this change isn't as critical, it just removes boilerplate code. When you look at IteratorChecker, SecuritySyntaxChecker or RetainCountBase they all use a similar, some cases faulty implementation to keep track of which subcheckers are enabled and what their name is, so its about time we combined them.

I envision this interface to be used to enforce our currently non-existent specification on the checker system.

In detail:

  • Introduce SuperChecker:
    • It is essentially the same as Checker, but requires an enum template argument to keep track of which subcheckers are enabled.
    • While previously we defined subcheckers as checkers that don't have a checker object on their own, SuperChecker does create a CheckerBase object for them, but they still can't have checker callbacks.
  • Add CheckerManager::registerSubChecker adds a new checker to a SuperChecker. It is ensured runtime that the SuperChecker was previously registered, and that a subchecker isn't registered multiple times.
  • Add thorough test cases for the new interface.

Diff Detail

Event Timeline

Szelethus created this revision.Sun, Sep 8, 3:08 PM
NoQ added a comment.Mon, Sep 9, 6:55 PM

I have mixed feelings. Removing boilerplate is good, but the very fact that we're legalizing this pattern indicates that our checkers will keep bloating up, while i always wanted to actually split them instead (like, make sub-checkers into their own separate classes, possibly spread out into different files, kinda micro checkers as opposed to monolithic checkers (?)). But i guess it's about whoever gets things done first :)

I'd love to see how this affects our actual checkers, did you already try porting them? Do you plan to help with tracking per-sub-checker bug types and check names?

SuperChecker

WDYT about MultiChecker? ("A checker that implements multiple checks and presents them as different checkers.")

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
113

The CXX23ModelingDiagKind:: qualifier is unnecessary here, right? Or did you mean to make an enum class? Does it even work with enum classes?