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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Why is there such a large jump in the number of warnings reported in the last patch iteration?
It went from "1678 projects where scanned. In total I got 124 warnings" to "In 2215 projects it found 875 warnings." Did the number of warnings in the initial 1678 projects stay the same?

Is it possible to take a look at the nature of the false positives, as per NoQ's request above?

This checker would benefit greatly from explaining why the errors occur. For example, where the values of variables are being constrained. Other checkers use BugReporterVisitor to generate the rich diagnostic information. Dow you have plans on how to accomplish that for this checker?


================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:11
@@ +10,3 @@
+// This defines ConversionChecker that warns about dangerous conversions where
+// there is possible loss of precision.
+//
----------------
Please, make this more specific.
Would be useful to summarize which heuristics you used to bring the number of false positives down.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:29
@@ +28,3 @@
+public:
+  void checkPreStmt(const ImplicitCastExpr *Cast, CheckerContext &C) const {
+    // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
----------------
nit: Please move the body out of the declaration.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:74
@@ +73,3 @@
+    if (!BT)
+      BT.reset(new BuiltinBug(this, "Conversion", "loss of sign/precision."));
+
----------------
Please, capitalize the sentence.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:84
@@ +83,3 @@
+// Can E value be greater or equal than Val?
+static bool canBeGreaterEqual(CheckerContext &C, const Expr *E,
+                              unsigned long long Val) {
----------------
This function returns true if the value "is" greater or equal, not "can be" greater or equal. The latter would be "return StGE".

Also, it's slightly better to return the StGE state and use it to report the bug. This way, our assumption is explicitly recorded in the error state.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:147
@@ +146,3 @@
+  if (canBeGreaterEqual(C, Cast->getSubExpr(), MaxVal))
+    reportBug(C, "loss of precision in implicit conversion");
+}
----------------
Please, use capitalization and punctuation.


http://reviews.llvm.org/D13126