[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Let's also have the link to your most recent mail about this issue here: http://lists.llvm.org/pipermail/cfe-dev/2018-November/060038.html

I re-read the mailing list conversation from 2016, @szepet's `MisusedMoveChecker` patch, so I have a better graps on whats happening.

I think this really is non-trivial stuff. When I initially read your workbook, `SymbolReaper` (along with what inlining really is) was among the biggest unsolved mysteries for me. The explanation of it you wrote back in 2016[1] is //awesome//, I feel enlightened after reading it, it's very well constructed, but I find it unfortunate that it took me almost a year of working in the analyzer to find it. After looking at `SymbolManager.cpp`'s history, I can see that the logic didn't change since, I'd very strongly prefer to have this goldnugget copy-pasted even to a simple txt file, and have it commited with this patch.

[1] http://lists.llvm.org/pipermail/cfe-dev/2016-March/048142.html

In https://reviews.llvm.org/D18860#1306359, @Szelethus wrote:

> In https://reviews.llvm.org/D18860#1297742, @NoQ wrote:
>
> > Nope, unit tests aren't quite useful here. I can't unit-test the behavior of API that i'm about to //remove//... right?
>
>
> Hmmm, can't really argue with that, can I? :D


Well, we probably should implement unit tests for these anyways, `SymbolReaper` is not only performance-critical, but is also among the few data structures that's an essential part of the analysis, and it's change in functionality could be very easily shown in dedicated test files. But that's an issue for another time, and until then, `debug.ExprInspection` still does the job well enough. Let's not delay this patch longer just because of that, and enjoy the post zombie apocalypse analyzer.


https://reviews.llvm.org/D18860