[PATCH] D19866: [Analyzer] Correct stack address escape diagnostic

[Devin Coughlin via cfe-commits]

dcoughlin added inline comments.

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:229
@@ -228,3 +228,3 @@
     BT_stackleak.reset(
-        new BuiltinBug(this, "Stack address stored into global variable",
-                       "Stack address was saved into a global variable. "
+        new BuiltinBug(this, "Stack address stored into global/static variable",
+                       "Stack address was saved into a global/static variable. "
----------------
zaks.anna wrote:
> I don't like the '/' here. The only idea I have is to replace it with "into a variable with static allocation", which is also not ideal because it uses jargon.
We have to be careful about changing the 'Name' parameter here. This is supposed to be stable, because we use it for issue hashing. Any change will cause the issue hash to change and stymie use of the issue hash for baselining/issue suppression. 

I think we should probably not change it. It isn't user facing, so improving the text won't make a difference in user experience (although to does show up in plists as the "type" of the bug (via the "BugType" field in PathDiagnostic).







================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:232
@@ -231,3 +231,3 @@
                        "This is dangerous because the address will become "
                        "invalid after returning from the function"));
 
----------------
This string here is actually never used, so you should just remove it and use the constructor of BuiltinBug that doesn't take a description.

================
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:242
@@ +241,3 @@
+      os << "static";
+    else
+      os << "global";
----------------
This is great!


http://reviews.llvm.org/D19866