[PATCH] D13126: New static analyzer checker for loss of sign/precision

[Artem Dergachev via cfe-commits]

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