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

[Devin Coughlin via Phabricator via cfe-commits]

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