The key is CXXRecordDecl::isDerivedFrom().
Details
Diff Detail
Event Timeline
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp | ||
---|---|---|
216 | That could be helpful, but I am not sure. | |
244–245 | Previously possible false positives happened because we kept flow the modeling. | |
clang/lib/StaticAnalyzer/Core/DynamicType.cpp | ||
46 | Probably a consistent way of MemRegion handling is more appropriate. |
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeInfo.h | ||
---|---|---|
25 | Ty(Ty) is the idiom here. | |
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp | ||
153 | Counterexample: struct A {}; struct B : A {}; struct C : A, B {}; Downcast from C to B should succeed, even though they have a common ancestor A (which has the same CXXRecordDecl but currently isn't the same object within C, but can be, if B declares A as a virtual base). | |
201 | Omg lol nice. Did you try to figure out how do other people normally do it? |
clang/lib/StaticAnalyzer/Core/DynamicType.cpp | ||
---|---|---|
46 | Pointer casts are quite a can of worms. I suggest ignoring it for now and dealing with it as needed. I.e., your code looks good... for now. |
clang/lib/StaticAnalyzer/Core/DynamicType.cpp | ||
---|---|---|
46 | *cries in pointer chasing* |
- Fix.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeInfo.h | ||
---|---|---|
25 | Good to know, thanks! | |
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp | ||
153 | So, even it is some kind of anti-pattern as a warning arrive immediately, now I allow B to C downcasts. Could you explain me more about that virtual idea, please? Based on this possible use-case in my mind two classes are on the same level as every of their bases/vbases are equals. | |
201 | There is no function for that in ADT/StringExtras.h + grep did not help, so I realized it is a common way to match vowels. Do you know a better solution? |
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp | ||
---|---|---|
154 | Using a flag here is unnecessary because you're returning true without doing anything else whenever the flag is set. So, basically, your code says "if at least one current base is different from at least one previous base, the cast succeeds". In particular, this function would return true whenever CurrentRD and PreviousRD are from completely unrelated class hierarchies. It sounds like whatever isSucceededDowncast() was supposed to do, it's not doing it right. | |
201 | I just realized that this is actually incorrect and the correct solution is pretty hard to implement because the actual "a vs. an" rule deals with sounds, not letters. Eg.:
has an "an" because it's read as "an el-el-vee-am". |
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp | ||
---|---|---|
153 |
In the following variation: struct A {}; struct B : virtual A {}; struct C : A, B {}; we have C contain only one instance of A: the layout of B within C would merely refer to the instance of A within C instead of making its own copy. In particular, the constructor of B within the constructor of C would not initialize C's instance of A because the constructor of C will invoke the constructor of A directly (cf. D61816). |
clang/lib/StaticAnalyzer/Core/DynamicType.cpp | ||
---|---|---|
99 | Hmm, I think this function answers the question, at least in the standard library sense, whether Y std::is_base_of of X, whereas "is derived from?" is another term that is used by clang and seems to not allow equality of types. Should we rename this to isBaseOf? |
- When we have no information whether the cast succeeds or not we assume both.
- CastVisitor is the new facility which decides whether the assumptions are appropriate.
- Removed the vowels-related string-formatting.
- Moved the API changes to D68199.
- The math is still WIP.
clang/lib/StaticAnalyzer/Core/DynamicType.cpp | ||
---|---|---|
99 | I have left the equality by a mistake. It is wanted to be a wrapper of isDerivedFrom(), which does not allow equality. Thanks for the notice! |
You need the math to decide whether delaying the decision to a visitor is the right approach.
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | Why is (isa<B>(a) && isa<C>(a)) deemed possible in the first test but not in the second test? o_o |
Yes, Soon(tm).
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | 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. |
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | 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"). |
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | 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). |
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | 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. |
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | 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. |
clang/test/Analysis/cast-value-hierarchy-fp-suppression.cpp | ||
---|---|---|
25–27 ↗ | (On Diff #222370) | 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. |
Ty(Ty) is the idiom here.