NoQ added inline comments.
================
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.
----------------
vsavchenko wrote:
> 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`
I had a look and it looks like all of our UIs are acting up quite a bit on this
example. The user ends up completely convinced that it's the current value of
`Original` that's getting leaked, not the original value.
Let's at least add a test and/or a comment? This definitely isn't a regression
and the patch is still really good.
================
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>
----------------
vsavchenko wrote:
> 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? 😅
Uh-oh ok fair enough :D
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100839/new/
https://reviews.llvm.org/D100839
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
