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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to