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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
