pgousseau added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:825
@@ -816,1 +824,3 @@
+ProgramStateRef CStringChecker::IsFirstBufInBound(CheckerContext &C,
+ ProgramStateRef state,
----------------
dcoughlin wrote:
> 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.
Yes returning a bool would be better thanks, I did not meant for this function
to add the assumption that the buffer is valid.
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:840
@@ +839,3 @@
+ Optional<NonLoc> Length = LengthVal.getAs<NonLoc>();
+ if (!Length)
+ return state;
----------------
dcoughlin wrote:
> 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?
>
>
Yes most of those checks seems redundant and/or might need to return
nullptr/false, will double check thanks.
================
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>()) {
+
----------------
dcoughlin wrote:
> Can you rewrite this if/else to avoid the indentation? (See
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return).
Yes thanks for the link !
================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:866
@@ +865,3 @@
+ assert(ER->getValueType() == C.getASTContext().CharTy &&
+ "CheckLocation should only be called with char* ElementRegions");
+
----------------
dcoughlin wrote:
> "CheckLocation" is copy pasta here.
Will fix !
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1115
@@ +1114,3 @@
+ // or have a symbolic offset.
+ if (SuperR) {
+ if (const ClusterBindings *C = B.lookup(SuperR)) {
----------------
dcoughlin wrote:
> 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;
Makes sense thanks.
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1125
@@ +1124,3 @@
+ ((*ROffset >= LowerOffset && *ROffset < UpperOffset) ||
+ (LowerOffset == UpperOffset && *ROffset == LowerOffset)))) {
+ B = B.removeBinding(I.getKey());
----------------
dcoughlin wrote:
> Please add a comment here to say that this is to handle arrays of 0 elements
> and arrays of 0-sized elements.
Will do !
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1132
@@ +1131,3 @@
+ if (R)
+ if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R))
+ VisitBinding(V);
----------------
dcoughlin wrote:
> If you are not going to use the result, you can use isa<SymbolicRegion>(R)
> here instead of dyn_cast.
Will do !
================
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}}
----------------
dcoughlin wrote:
> Should this test m->s3[j] as well?
Yes that is what I meant thanks !
================
Comment at: test/Analysis/pr22954.c:498
@@ +497,3 @@
+}
+
+
----------------
dcoughlin wrote:
> Can you also add a test where the size argument to memcpy is the result of
> sizeof()?
Will do !
http://reviews.llvm.org/D11832
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
