NoQ added a comment.
In D83961#2166903 <https://reviews.llvm.org/D83961#2166903>, @balazske wrote:
> Every other test failure comes from RetainCount checker except
> //malloc-plist.c//.
Aha, ok. So, anyway, for retain count checker we ultimately only care about
plist and html reports, not about text reports. It's also probably easier to
write more precise tests after your patch: test functions are unlikely to have
multiple possible uniqueing locations, and even if there are they can be
discriminated between by warning message text.
But i'd still rather have it explained why does your patch affect the location
of `RefLeakReport`s in order to make sure we understand what we're doing. This
consequence of the patch wasn't intended - what other unintended consequences
does it have? Are we even sure that plist/html reports don't change? Is
`RefLeakReport` even implemented correctly from our current point of view? Or,
regardless of correctness, do we want its current implementation to have the
old behavior or the new behavior?
I should probably investigate this myself from the fairness/justice point of
view but you'll probably land your patch faster if you don't wait on me to get
un-busy with other stuff :/ As a bonus, you might be able to get away without
updating all the tests if you find out that this change is accidental and not
really intended :) Note that you don't need to know Objective-C or know much
about the checker to investigate these things; say, CGColorSpace.c is a pure C
test with a straightforward resource leak bug. The customizations in
`RefLeakReport` aren't really checker-specific either - we simply didn't ever
make up our mind on whether they should apply to all leak reports or to none;
they have visual consequences on plist reports though.
================
Comment at: clang/test/Analysis/malloc-plist.c:137-139
if (y)
- y++;
-}//expected-warning{{Potential leak}}
+ y++; //expected-warning{{Potential leak}}
+}
----------------
balazske wrote:
> NoQ wrote:
> > This sounds like an expected change: we're now displaying the same report
> > on a different path. Except it's the longer path rather than the shorter
> > path, so it still looks suspicious.
> This location may be wrong but it is then another problem. The important
> thing is that after this patch the same location is used for a warning in
> `text-minimal` mode as it is in `text` mode. Without the patch in
> `text-minimal` mode a different location is used for this warning, the one at
> the end of the function. But still in `text` mode (without the patch) the
> location at `y++` is shown. (The warning in function `function_with_leak4` in
> the same test is already at a similar location, not at the end of function.)
Oh, wait, the other path isn't in fact shorter. In both cases it leaks
immediately after the if-statement, and the path with `y++` is in fact a bit
shorter (it immediately hits `PreStmtPurgeDeadSymbols` for the `DeclRefExpr`
whereas the other path hits `CallExitBegin` first, because the call is inlined).
So it's a good and expected change!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83961/new/
https://reviews.llvm.org/D83961
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
