[PATCH] D66423: [analyzer] CastValueChecker: Model isa(), isa_and_nonnull()

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:183
+  if (Body)
+    DRE = dyn_cast<DeclRefExpr>(Body);
+
----------------
A body of a function is always either a `CompoundStmt` or (shockingly) a `CXXTryStmt`. This cast will always fail.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:188
+
+  QualType CastToTy = DRE->getTemplateArgs()->getArgument().getAsType();
+  QualType CastFromTy = getRecordType(Call.parameters()[0]->getType());
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:216
+  const NoteTag *Tag = C.getNoteTag(
+      [=](BugReport &) -> std::string {
+        SmallString<128> Msg;
----------------
Can you use the overload without the bug report parameter? Cause you don't seem to be using it anyway.


================
Comment at: clang/test/Analysis/cast-value-notes.cpp:5-27
 namespace llvm {
 template <class X, class Y>
 const X *cast(Y Value);
 
 template <class X, class Y>
 const X *dyn_cast(Y *Value);
 template <class X, class Y>
----------------
Szelethus wrote:
> 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 :)
I agree, extracting common definitions into a system header simulator usually ends up being worth it pretty quickly.


================
Comment at: clang/test/Analysis/cast-value-notes.cpp:89
+  if (isa<Triangle>(C)) {
+    // expected-note at -1 {{'C' with type 'Circle' is not the instance of 'Triangle'}}
+    // expected-note at -2 {{Taking false branch}}
----------------
I suggest: `'C' is a 'Circle', not a 'Triangle'`.


================
Comment at: clang/test/Analysis/cast-value-notes.cpp:95
+  if (isa<Circle>(C)) {
+    // expected-note at -1 {{'C' with type 'Circle' is a 'Circle'}}
+    // expected-note at -2 {{Taking true branch}}
----------------
Just `'C' is a 'Circle'`.


================
Comment at: clang/test/Analysis/cast-value-notes.cpp:111
+  if (isa<Triangle>(C)) {
+    // expected-note at -1 {{Assuming 'C' with type 'Circle' is not the instance of 'Triangle'}}
+    // expected-note at -2 {{Taking false branch}}
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66423