dcoughlin added a comment.
Artem: Sorry it too me so long to review this! For CXXStdInitializerListExpr
this looks great. However, I don't think we want ObjCBoxedExprs to escape their
arguments. It looks to me like these expressions copy their argument values and
don't hold on to them.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1127
+ // only consist of ObjC objects, and escapes of ObjC objects
+ // aren't so important (eg., retain count checker ignores them).
+ if (isa<CXXStdInitializerListExpr>(Ex) ||
----------------
Note that we do have other ObjC checkers that rely on escaping of ObjC objects,
such as the ObjCLoopChecker and ObjCDeallocChecker. I think having the TODO is
great, but I'd like you to remove the the bit about "escapes of ObjC objects
aren't so important".
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1129
+ if (isa<CXXStdInitializerListExpr>(Ex) ||
+ (isa<ObjCBoxedExpr>(Ex) &&
+ cast<ObjCBoxedExpr>(Ex)->getSubExpr()->getType()->isRecordType()))
----------------
In general, I don't think we want things passed to ObjCBoxedExpr to escape. The
boxed values are copied and encoded when they are boxed, so the boxed
expression doesn't take ownership of them.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1132
+ for (auto Child : Ex->children()) {
+ if (!Child)
+ continue;
----------------
Is this 'if' needed?
================
Comment at: test/Analysis/objc-boxing.m:66
+ BoxableStruct bs;
+ bs.str = strdup("dynamic string"); // The duped string shall be owned by val.
+ NSValue *val = @(bs); // no-warning
----------------
In this case the duped string is not owned by `val`. NSValue doesn't take
ownership of the string, so this *will* leak and we should warn about it.
https://reviews.llvm.org/D35216
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
