[PATCH] D65889: [analyzer] CastValueChecker: Model castAs(), getAs()

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:51
+
+  const CallDescriptionMap<CastCheck> SugarCastCDM = {
+      {{{"clang", "castAs"}, 0}, &CastValueChecker::evalCastAs},
----------------
I'd rather have only one map from call descriptions to (callback, kind) pairs. This should make the `evalCall` code much more readable.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:215
   const auto *CE = cast<CallExpr>(Call.getOriginExpr());
-  if (!CE)
+  if (!CE->getType()->getPointeeCXXRecordDecl())
     return false;
----------------
Use `Call.getResultType()` instead.

And please add a test with references, as `getResultType` behaves more correctly for references:

```lang=c++
C &c = ...;
D &d = dyn_cast<D>(c);
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:223
+    // If we cannot obtain the arg's class we cannot be sure how to model it.
+    if (!CE->getArg(0)->getType()->getPointeeCXXRecordDecl())
+      return false;
----------------
`*Call.parameters()[0]->getType()`. It should also help with references. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:231
+    // If we cannot obtain 'MCE' we cannot be sure how to model it.
+    const auto *MCE = dyn_cast<CXXMemberCallExpr>(CE->IgnoreParenImpCasts());
+    if (!MCE)
----------------
```lang=c++
const auto *InstanceCall = dyn_cast<CXXInstanceCall>(&Call);
...
DV = InstanceCall->getCXXThisVal();
```


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:718
+  assert(ThisVal.isUnknownOrUndef() || ThisVal.getAs<Loc>() ||
+         ThisVal.getAs<nonloc::ConcreteInt>()->getValue().getExtValue() == 1);
   return ThisVal;
----------------
I don't think you need this anymore.


================
Comment at: clang/test/Analysis/cast-value.cpp:186
+  // expected-note at -1 {{Assuming pointer value is null}}
+  // expected-note at -2 {{Assuming dynamic cast to 'Circle' succeeds}}
+  // expected-note at -3 {{'C' initialized here}}
----------------
To think: this note isn't very useful because hard casts aren't really supposed to fail anyway.


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

https://reviews.llvm.org/D65889