[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Thanks! I like where this is going. Let's land the patch and continue developing it incrementally in trunk.

The next steps for this checker are, in my opinion:

- Do the visitor thingy that i requested in inline-311373 <https://reviews.llvm.org/D33672#inline-311373>. I think it's a necessary thing to do, but don't jump into implementing it right away: i already have some code for this that i want to share.
- Play nicely with typedefs. For now i believe the checker ignores them because you can't cast `TypedefType` to `EnumType`. Once this is done, it will be worth it to include the name of the enum in the warning message.
- Optimize the code using `assumeInclusiveRange`. Because `assume` is an expensive operation, i'd like to avoid doing it O(n) times for contiguous enums in which just 2 `assume`s are enough (or, even better, as single `assumeInclusiveRange`).
- See how this checker performs on real code, fix crashes and false positives if any.



================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:36
+  const ProgramStateRef PS;
+  SValBuilder &SVB;
+
----------------
ZaMaZaN4iK wrote:
> Szelethus wrote:
> > You can acquire `SValBuilder` from `ProgramState`:
> > `PS->getStateManager()->getSvalBuilder()`
> Is there any difference? Is it critical to get `SValBuilder` from `CheckerContext' ?
There's only one instance of `SValBuilder` in existence at any particular moment of time. The same applies to `BasicValueFactory`, `SymbolManager`, `MemRegionManager`, `ConstraintManager`, `StoreManager`, `ProgramStateManager`, ...

All these objects live within `ExprEngine` and have the same lifetime.

`ExprEngine`, together with all these objects, is created from scratch for every analysis of a top-level function.

AST entities, such ast `ASTContex`, on the contrary, live much longer - only one is created per clang process. That is, until somebody takes `ASTImporter` and tries to frankenstein multiple ASTs into one :)


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:84-86
+          new BuiltinBug(this, "enum cast out of range",
+                         "the value provided to the cast expression is not in "
+                         "the valid range of values for the enum"));
----------------
> diagnostics should not be full sentences or grammatically correct, so drop the capitalization and full stop

No, in fact, Static Analyzer diagnostics are traditionally capitalized, unlike other warnings, so i'm afraid this will need to be changed back >.< Still no fullstop though.


https://reviews.llvm.org/D33672