NoQ added a comment.
I've got a few minor code comments.
I really wish to have a look at false positives on which
> the value analysis fails and then there is not much my checker could do
either in a form of FIXME tests, or as preprocessed code samples, because i'm
currently digging the topic of improving cast modeling (either through
producing `SymbolCast` more often, or by taking sub-symbol-types vs.
symbolic-expression-types vs. ast-expression-types into account during
`assume()` and `evalBinOp()` and various`ExprEngine` calls. Eg, when casting an
int into a char and then back into an int, ensure that the resulting int value
carries, at most, a char-range constraint. So i wonder if such fix would help.
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:78
@@ +77,3 @@
+
+// Can E value be greater or equal than Val?
+static bool canBeGreaterEqual(CheckerContext &C, const Expr *E,
----------------
It's not quite "the value can be greater or equal", but in fact rather "the
value is certainly greater or equal".
Same applies to `canBeNegative()`.
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:85
@@ +84,3 @@
+ return false;
+ DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>();
+
----------------
A slightly shorter way to express the same thing:
```
llvm::Optional<NonLoc> EVal = C.getSVal(E).getAs<NonLoc>();
if (!EVal)
return false;
// later use (*EVal)
```
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:96
@@ +95,3 @@
+ ProgramStateRef StGE = State->assume(GE.castAs<DefinedSVal>(), true);
+ ProgramStateRef StLT = State->assume(GE.castAs<DefinedSVal>(), false);
+ return (StGE && !StLT);
----------------
You can use the overridden version of `assume()` that returns two states at
once into a std::pair, in order to avoid double-work.
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:121
@@ +120,3 @@
+
+static bool isConstant(const Expr *E, const ASTContext &Ctx) {
+ E = E->IgnoreParenCasts();
----------------
Not sure, but shouldn't the common `Expr::EvaluateAsInt()` mechanism handle it
all of it, including recursive traversal through operators?
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:142
@@ +141,3 @@
+
+ if (!SubType.getTypePtr()->isIntegerType() ||
+ !ResultType.getTypePtr()->isIntegerType())
----------------
You don't need to call `.getTypePtr()`, because `QualType` already has a
convenient overridden `operator ->()`, which gives you quick access to the
underlying `Type *`.
================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:158
@@ +157,3 @@
+
+ unsigned long long MaxVal = 1ULL << W;
+ if (canBeGreaterEqual(C, Cast->getSubExpr(), MaxVal))
----------------
`BasicValueFactory::getMaxValue(QualType)` might be useful.
http://reviews.llvm.org/D13126
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
