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@-4 {{Taking true branch}}
+ (void)(1 / !Baz);
+ // expected-note@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
