NoQ added a comment.

Oh, these false positives! Thanks!!



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1627
+                                    svalBuilder.makeIntVal(1, sizeTy), sizeTy);
+          Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>();
+
----------------
Please handle the situation when `freeSpaceNL` is `None` before dereferencing. 
I suspect that this might actually happen when you overwhelm the symbol 
complexity by subtracting 1. I'm not really super sure this can happen, given 
that symbols we're working with are very specific as of now, but this is 
something that should be made possible if we ever go far enough in our attempts 
to improve this checker.


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1637
+          if (TrueState && !FalseState) {
+            // srcStrLength <=  size - dstStrLength -1
+            amountCopied = strLength;
----------------
Strange whitespace here and in the next comment (:


================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1647
+          if (TrueState && FalseState) {
+            amountCopied = UnknownVal();
+          }
----------------
Great job not introducing the state split here!


================
Comment at: test/Analysis/cstring-syntax.c:57
+
+  //test for Bug 41729
+  char a[256], b[256];
----------------
I think this test doesn't really demonstrate that bug 41729 is fixed, as the 
offending checker isn't enabled on this test.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66049/new/

https://reviews.llvm.org/D66049



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to