[PATCH] D100839: [analyzer] Adjust the reported variable name in retain count checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This is a major improvement. `MallocChecker` will have to catch up on that. Hopefully through increased code re-use.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:651
+
+  return Result;
+}
----------------
I feel semi-irrational urge to add comments whenever NRVO is employed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:967-968
+
+  // While looking for the last var bindings, we can still find
+  // `AllocFirstBinding` to be one of them.  In situations like this,
+  // it would still be the easiest case to explain to our users.
----------------
Given that the current state doesn't satisfy the requirement on line 945, this can happen for two reasons:
1. The symbol was there when the variable died so we simply tracked it back to the last moment of time the variable was alive.
2. The symbol was there but was overwritten later (and then the variable may have also, but haven't necessarily, died).

Case 2 is bad because we're back in business for reporting the wrong variable; it's still more rare than before the patch but i think the problem remains. Something like this should probably demonstrate it:

```lang=c++
void foo() {
  Object *Original = allocate(...);
  Original = allocate();
  Original->release();
}
```

A potential solution may be to add a check in `getAllVarBindingsForSymbol` that the first node in which we find our symbol corresponds to a `*PurgeDeadSymbols` program point. It's a straightforward way to find out whether we're in case 1 or in case 2.


================
Comment at: clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist:5993
+     <key>message</key>
+     <string>Object leaked: object allocated and stored into 'New' is not referenced later in this execution path and has a retain count of +1</string>
+    </dict>
----------------
This patch seems to add 3 entirely new warnings here. Why do they suddenly start showing up? Are they good?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100839/new/

https://reviews.llvm.org/D100839