[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

Kristóf Umann via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 7 10:30:48 PST 2018


Szelethus added a comment.

In D54438#1315953 <https://reviews.llvm.org/D54438#1315953>, @Szelethus wrote:

> - ❌ Move `CheckerManager::registerChecker<T>` out of the registry functions.
>   - ❌ Since `shouldRegister##CHECKERNAME` is a thing, we can move everything to the checker's constructor, supply a `CheckerManager`, eliminating the function entirely.
>   - ❌ At long last, get rid of `CheckerManager::setCurrentCheckerName` and `CheckerManager::getCurrentCheckerName`.
>     - ❌ It was discussed multiple times (D48285#inline-423172 <https://reviews.llvm.org/D48285#inline-423172>, D49438#inline-433993 <https://reviews.llvm.org/D49438#inline-433993>), that acquiring the options for the checker isn't too easy, as it's name will be assigned only later on, so currently all checkers initialize their options past construction. This can be solved either by supplying the checker's name to every constructor, or simply storing all enabled checkers in `AnalyzerOptions`, and acquire it from there. I'll see.


Pulling this off is not only difficult, certainly super-invasive, but also unnecessary -- in the final patch (D55429 <https://reviews.llvm.org/D55429>) I used a far simpler (~7 lines of code) solution, that still ensures that the checker naming problem never comes up again.

Thank you so much @NoQ for all the feedback! This project has been a super fun. I still expect some skeletons to fall out of the closet, but I'm fairly confident that the overall direction is set and is good.


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

https://reviews.llvm.org/D54438





More information about the cfe-commits mailing list