[PATCH] D64374: [analyzer] CastValueChecker: Model casts

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks! My mind was really set to actually model these with `classof()`, whoops.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:25-27
+  using CastCheck =
+      std::function<void(const CastValueChecker *, const CallExpr *,
+                         DefinedOrUnknownSVal, CheckerContext &)>;
----------------
NoQ wrote:
> Kinda nice, but a simple pointer-to-member would have been sufficient and much more lightweight:
> 
> ```lang=c++
> using CastCheck = void (CastValueChecker::*)(const CallExpr *,
>                          DefinedOrUnknownSVal, CheckerContext &);
> ```
> 
> (not sure, i don't fully understand the performance implications of using `std::function`)
It took me an hour to realize I have to push the class as the signature of the function to create a member-callback. I think it is not that lightweight, but it is modern. I believe it is made for that purpose when you could write `Foo[13]` or `*(13 + P)`, where the latter creeps me out.

Thanks for the copy-pasteable code, I really enjoy to see what do you think exactly, but this is the first time when I would pick my idea. (I would had recommended that modern way in your patch just I thought it is not working with methods.)


================
Comment at: clang/test/Analysis/cast-value.cpp:34
+    // expected-note at -4 {{Taking true branch}}
+    (void)(1 / !Baz);
+    // expected-note at -1 {{'Baz' is non-null}}
----------------
NoQ wrote:
> `debug.ExprInspection` is your friend!
> 
> I'd also call for making the tests more isolated. In this test you're hardcoding a fairly arbitrary and random execution path that the analyzer chose through your function. It's generally interesting what paths do we explore first, but that's not what the patch is about.
> 
> Here's an example of a test that i would have written:
> ```lang=c++
> void foo(Shape *S) {
>   Circle *C = dyn_cast_or_null<Circle>(S);
>   clang_analyzer_numTimesReached(); // expected-warning{{3}}
>   if (S && C)
>     clang_analyzer_eval(C == S); // expected-warning{{TRUE}}
>   if (!S)
>     clang_analyzer_eval(!C); // expected-warning{{TRUE}}
>   if (S && !C)
>     clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
> }
> ```
> (you might want to test notes separately)
Great idea, thanks!


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

https://reviews.llvm.org/D64374