[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.

Devin Coughlin via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Sep 28 18:19:39 PDT 2017

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.


More information about the cfe-commits mailing list