Charusso marked 4 inline comments as done. Charusso added inline comments.
================ Comment at: clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp:25-27 + if (isa<B>(a)) + if (isa<C>(a)) + clang_analyzer_warnIfReached(); // no-warning ---------------- NoQ wrote: > NoQ wrote: > > Charusso wrote: > > > NoQ wrote: > > > > Charusso wrote: > > > > > NoQ wrote: > > > > > > Why is `(isa<B>(a) && isa<C>(a))` deemed possible in the first test > > > > > > but not in the second test? o_o > > > > > In `test_downcast()` we assume that `a` is a record type of `D` where > > > > > `D` is a `B` and `D` is a `C`. However in > > > > > `test_downcast_infeasible()` if `a` is not a record type of `D` is > > > > > cannot be both `B` and `C` at the same time. That is the purpose of > > > > > `CastVisitor`. > > > > I mean, it contradicts to how the program *actually* works, so we > > > > should either not do that, or provide a reeeeeaaaaaallly compelling > > > > explanation of why we do this (as in "Extraordinary Claims Require > > > > Extraordinary Evidence"). > > > Are you sure it does not model the program? I have an `Apple` class and I > > > have a `Pen` class, until it is not defining an `ApplePen` class it is a > > > false assumption to say they are defining an `ApplePen` class. I wanted > > > to prefetch that information before the modeling starts, but it was an > > > impossible challenge for me, so I have picked that `CastVisitor`-based > > > post-elimination idea. In the real world I have removed only two false > > > assumptions with the visitor from 1200 reports of LLVM so an `ApplePen` > > > is very rare (https://www.youtube.com/watch?v=Ct6BUPvE2sM). > > I'm sure that the possibility of taking a true branch in `if (x)` only > > depends on the value of `x`, not on the contents of the branch. > I.e., i see the point that you're trying to make (roughly), but it still > requires extraordinary evindence and the way you've written the test clearly > demonstrates why it needs an extraordinary evidence. > > What you were trying to say is "imagine there's no class `D`, then we > probably shouldn't consider the situation that it exists". But in this test > the class exists, and you've just written a test about it, and then you're > telling me that it doesn't exist. Which is exactly the problem with not > considering that `D` exists. And this is exactly why choosing such approach > requires a discussion. > > One reason why i doubt the visitor-based solution is that the existence of > `D` is not a path-sensitive fact; it either exists or not, it is irrelevant > whether it was mentioned in the current path. Therefore scanning all classes > in the translation unit, as opposed to classes mentioned on the current path, > seems more precise. Well, that is a better idea, yes. I really like your observation about `if (x)`, thanks for them! During the Summer I have learnt a lot, but a lot about the issue of the one translation unit, so that is why I was very afraid of using it. Let us give it a try. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67079/new/ https://reviews.llvm.org/D67079 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
