[PATCH] D67079: [analyzer] CastValueChecker: Model inheritance

[Artem Dergachev via Phabricator via cfe-commits]

NoQ 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:
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67079/new/

https://reviews.llvm.org/D67079