[PATCH] D83961: [Analyzer] Fix bug report source locations in minimal output.

[Artem Dergachev via Phabricator via cfe-commits]

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