dcoughlin requested changes to this revision.
dcoughlin added a comment.
This revision now requires changes to proceed.
Thanks for the patch Ismail! Some comments inline.
================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:408
@@ +407,3 @@
+ if (!Base.isZeroConstant()) {
+ if (const FieldDecl *FD = dyn_cast<const FieldDecl>(D)) {
+ ASTContext &Ctx = D->getASTContext();
----------------
You can use `auto *FD = dyn_cast<const FieldDecl>(D)` here to avoid repeating
the type.
================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:410
@@ +409,3 @@
+ ASTContext &Ctx = D->getASTContext();
+ CharUnits CU = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FD));
+ loc::ConcreteInt BasePtr = BaseL.castAs<loc::ConcreteInt>();
----------------
`getFieldOffset()` will assert when the field is an Objective-C instance
variable because it expects the field's parent to be a record. For example:
`int *x = &((SomeClass *)0x10)->someIVar;`
================
Comment at: test/Analysis/array-struct-region.cpp:128
@@ -109,1 +127,3 @@
+#endif
+}
----------------
This test doesn't cover the new logic you added in Store.cpp because it is
never the case in this test that the base is a non-zero ConcreteInt Loc. In
addition to your test with `PFONull`, you should add tests with something like
`struct FieldOffset *PFOConstant = (struct FieldOffset *) 0x22;`. I think it is
also important to add tests for Objective-C instance variables after addressing
the assertion failure in that case.
http://reviews.llvm.org/D12251
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
