[PATCH] D39438: [analyzer] Diagnose stack leaks via block captures

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I found some nits, but overall I think this is getting close.



================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:76
     range = CL->getSourceRange();
-  }
-  else if (const AllocaRegion* AR = dyn_cast<AllocaRegion>(R)) {
+  } else if (const AllocaRegion *AR = dyn_cast<AllocaRegion>(R)) {
     const Expr *ARE = AR->getExpr();
----------------
In case you already change these lines you could replace the type name on the left side with `auto` to avoid repeating types. 


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:123
+  for (const auto &C : B.captures()) {
+    const TypedefType *T = C.getVariable()->getType()->getAs<TypedefType>();
+    if (T && T->getDecl()->getIdentifier() == dispatch_semaphore_tII)
----------------
You can also use auto here to avoid mentioning the type twice. 


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:192
+          this, "Address of stack-allocated memory is captured");
+    SmallString<512> Buf;
+    llvm::raw_svector_ostream Out(Buf);
----------------
How long usually these error messages are? Maybe 512 is a bit large buffer for this? Note that in case the error message is longer this is still not a hard error, it just will hit the slow path (allocating the buffer on the heap).


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:214
+      BT_capturedstackret = llvm::make_unique<BuiltinBug>(
+          this, "Address of stack-allocated memory");
+    SmallString<512> Buf;
----------------
Maybe you wanted to mention "captured" here as well?


================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:215
+          this, "Address of stack-allocated memory");
+    SmallString<512> Buf;
+    llvm::raw_svector_ostream Out(Buf);
----------------
Maybe the error reporting part could be abstracted out to avoid code duplication with the function above?


Repository:
  rL LLVM

https://reviews.llvm.org/D39438