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

[Artem Dergachev via cfe-commits]

NoQ added a comment.

Wow, i've completely forgot about this one. Sorry about that... I think this checker is good to have in the current shape, and probably incrementally developed. It'd probably move out of alpha whenever it's tweaked to somehow make sure it finds enough real bugs among intentional integer truncations, which would most likely require some nontrivial heuristics.

Added a few very minor comments, once they're fixed i'd consider committing(?)


================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:10
@@ +9,3 @@
+//
+// Check that there is not loss of sign/precision in assignments, comparisons
+// and multiplications.
----------------
~~not~~ => no

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:21
@@ +20,3 @@
+// Many compilers and tools have similar checks that are based on semantic
+// analysis. Those checks are sound but has poor precision. ConversionChecker
+// is an alternative to those checks.
----------------
~~has~~ => have

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:66
@@ +65,3 @@
+  const Stmt *Parent = PM.getParent(Cast);
+  if (!Parent)
+    return;
----------------
I think you can assert that the parent exists here.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:87
@@ +86,3 @@
+  // Generate an error node.
+  ExplodedNode *N = C.generateErrorNode(C.getState());
+  if (!N)
----------------
You are generating a fatal error node, which stops the analysis on the path on which the bug is found. I don't think it's desired, because the program isn't known to terminate upon loss of sign/precision, and it suppresses other warnings by other checkers. I think you need to use `generateNonFatalErrorNode()` here. But then you need to make sure you generate this node exactly once per callback and re-use for all reports in case there are multiple reports, otherwise the execution path behind this node may be duplicated.


https://reviews.llvm.org/D13126