dcoughlin added a comment.
Thanks for adding handling of the symbolic cases! Some more comments inline.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:825
@@ -816,1 +824,3 @@
+ProgramStateRef CStringChecker::IsFirstBufInBound(CheckerContext &C,
+ ProgramStateRef state,
----------------
It seems odd that you return a ProgramStateRef here but don't use it in the
only caller of this function. Could you just return a boolean instead?
Alternatively, if you really want to add the assumption that the buffer is
valid to the post-state, then you should thread the state returned from the
call to this function through the rest of CStringChecker::InvalidateBuffer.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:840
@@ +839,3 @@
+ Optional<NonLoc> Length = LengthVal.getAs<NonLoc>();
+ if (!Length)
+ return state;
----------------
You have a lot of early returns of the form:
if (!Foo)
return state;
that don't seem to be exercised in your tests. Can these actually trigger? If
so, you should add tests for these cases. If not, maybe
asserts/cast<T>()/castAs<T>() would be more appropriate.
Also: should these early returns return state? Or should they return null?
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:851
@@ +850,3 @@
+ SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType());
+ if (Optional<Loc> BufLoc = BufStart.getAs<Loc>()) {
+
----------------
Can you rewrite this if/else to avoid the indentation? (See
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return).
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:866
@@ +865,3 @@
+ assert(ER->getValueType() == C.getASTContext().CharTy &&
+ "CheckLocation should only be called with char* ElementRegions");
+
----------------
"CheckLocation" is copy pasta here.
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1115
@@ +1114,3 @@
+ // or have a symbolic offset.
+ if (SuperR) {
+ if (const ClusterBindings *C = B.lookup(SuperR)) {
----------------
This nesting is getting pretty deep. Can you invert some guards and change to
goto/continue where appropriate.
For example:
if (!SuperR)
goto conjure_default;
and
const ClusterBindings *C = B.lookup(SuperR)
if (!C)
continue;
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1125
@@ +1124,3 @@
+ ((*ROffset >= LowerOffset && *ROffset < UpperOffset) ||
+ (LowerOffset == UpperOffset && *ROffset == LowerOffset)))) {
+ B = B.removeBinding(I.getKey());
----------------
Please add a comment here to say that this is to handle arrays of 0 elements
and arrays of 0-sized elements.
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1132
@@ +1131,3 @@
+ if (R)
+ if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R))
+ VisitBinding(V);
----------------
If you are not going to use the result, you can use isa<SymbolicRegion>(R) here
instead of dyn_cast.
================
Comment at: test/Analysis/pr22954.c:476
@@ +475,3 @@
+ clang_analyzer_eval(m->s3[3] == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(m->s3[i] == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(l->s1[0] == 1); // expected-warning{{UNKNOWN}}
----------------
Should this test m->s3[j] as well?
================
Comment at: test/Analysis/pr22954.c:498
@@ +497,3 @@
+}
+
+
----------------
Can you also add a test where the size argument to memcpy is the result of
sizeof()?
http://reviews.llvm.org/D11832
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
