Page MenuHomePhabricator

[analyzer] CastValueChecker: Model isa(), isa_and_nonnull()
ClosedPublic

Authored by Charusso on Aug 19 2019, 9:02 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Charusso created this revision.Aug 19 2019, 9:02 AM

Whoa, the test cases look AMAZING! I admit that I haven't read much of the other code just yet, but I like the results in any case.

clang/test/Analysis/cast-value-notes.cpp
5–27 ↗(On Diff #215908)

Hmm, we may want to move these eventually to clang/test/Inputs/, since this isn't the only LLVM checker. Thought that said, I want to see the other LLVM checker gone, like, purged from the codebase for being the abomination that it is.

Feel free to leave these here for know, just thinking aloud :)

NoQ added inline comments.Aug 19 2019, 3:22 PM
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
183 ↗(On Diff #215908)

A body of a function is always either a CompoundStmt or (shockingly) a CXXTryStmt. This cast will always fail.

188 ↗(On Diff #215908)

I suspect that DRE may still be null (eg., when calling isa through function pointer).

I think you should just lookup Call.getDecl()'s template arguments instead. It's going to be the declaration of the specific instantiation, so it'll have all the arguments.

216 ↗(On Diff #215908)

Can you use the overload without the bug report parameter? Cause you don't seem to be using it anyway.

clang/test/Analysis/cast-value-notes.cpp
5–27 ↗(On Diff #215908)

I agree, extracting common definitions into a system header simulator usually ends up being worth it pretty quickly.

89 ↗(On Diff #215908)

I suggest: 'C' is a 'Circle', not a 'Triangle'.

95 ↗(On Diff #215908)

Just 'C' is a 'Circle'.

111 ↗(On Diff #215908)

This test looks incorrect. We shouldn't be assuming this, we already know that it's not a triangle because we know that it's a circle.

Charusso marked 9 inline comments as done.Aug 20 2019, 2:04 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
216 ↗(On Diff #215908)

I think you have forgotten to implement that scenario, but now we have that as well since D66325.

clang/test/Analysis/cast-value-notes.cpp
5–27 ↗(On Diff #215908)

Thanks for the idea! Moved in D66325.

111 ↗(On Diff #215908)

Let us discuss in D66325.

Charusso updated this revision to Diff 216322.Aug 20 2019, 7:46 PM
Charusso marked 7 inline comments as done.
  • Simplify the template argument obtaining.
  • Added a tiny new test case.

Thanks for the reviews so far! I had a contradiction in my mind about regions, but now everything is okay and the notes are not misleading.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
183 ↗(On Diff #215908)

I do not remember what I did here, but that was a working solution.

188 ↗(On Diff #215908)

That was the plan, just I did not realized I am a FunctionDecl away to achieve that. Thanks!

NoQ accepted this revision.Aug 21 2019, 11:12 AM

LGTM with one minor nit!

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
233–235 ↗(On Diff #216322)

Mmm, this will need to be de-duplicated out.

This revision is now accepted and ready to land.Aug 21 2019, 11:12 AM

Thanks for the review!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 7:59 PM