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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:651
+
+  return Result;
+}
----------------
NoQ wrote:
> I feel semi-irrational urge to add comments whenever NRVO is employed.
I know, and I lacked trust in compilers that all of them will do it, but I think things like this should not be commented. It's not part of main the logic and it looks natural, so we can not explain why we did it this way in particular.


================
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.
----------------
NoQ wrote:
> 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.
In this situation, we will report on line `Original = allocate();`, which should probably be a good enough indicator of what analyzer tries to say.
In your example, we have a choice of either not report variable name at all or report `Original`


================
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>
----------------
NoQ wrote:
> This patch seems to add 3 entirely new warnings here. Why do they suddenly start showing up? Are they good?
Because I added 3 more test cases? 😅


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100839