[PATCH] D12571: [Analyzer] Fix assertions in commit r246345 (pr22954).

[pierre gousseau via cfe-commits]

pgousseau added inline comments.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:843
@@ +842,3 @@
+  if (!Length)
+    return true;
+
----------------
dcoughlin wrote:
> There doesn't seem to be a test that cares about this returning true (as compared to false).
Will add thanks, f263 was the test meant for it.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:855
@@ +854,3 @@
+  if (!BufLoc)
+    return true;
+
----------------
dcoughlin wrote:
> There doesn't appear to be a test that cares about this returning true (as compared to false).
Will add thanks, f34 was the test meant for it.

================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:863
@@ +862,3 @@
+  if (!R)
+    return true;
+
----------------
dcoughlin wrote:
> What's the rationale for treating the array access as in-bounds if the BufEnd is unknown? Or if LengthValue is unknown? Should these branches return false? Either way, can you add a comment explaining why this is the right thing to do and also update the doc comment of IsFirstBufInBound to reflect this behavior (e.g., "Returns true if destination buffer of copy function must/may be in bound")
Yes I mean to return true in the case of an unknown array access to not warn on unknown state, will add comment thanks.

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1109
@@ +1108,3 @@
+      }
+      assert(RO.getOffset() >= 0 && "Offset should not be negative");
+      uint64_t LowerOffset = RO.getOffset();
----------------
dcoughlin wrote:
> This assertion is triggering on our internal build bots. I'm working to get a reduced test case.
I can replace the assert by an if statement meanwhile ?

================
Comment at: test/Analysis/pr22954.c:739
@@ +738,3 @@
+// Test tainted values.
+struct yy {
+  char s1[4];
----------------
dcoughlin wrote:
> A question: how does this test tainted values?
Yes this comment is wrong, this is not testing tainted values. Will change comment and description thanks.


http://reviews.llvm.org/D12571