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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Looking good now! I still recommend eventually adding tests with casts of references.

In D65889#1620128 <https://reviews.llvm.org/D65889#1620128>, @Charusso wrote:

> - May it is better to also check for `getAsCXXRecordDecl()` for obtaining a class.




In D65889#1621587 <https://reviews.llvm.org/D65889#1621587>, @Charusso wrote:

> - The call as `getAsCXXRecordDecl()` sometimes run into an assertion failure, so we need to avoid it


Well, yeah, i was just about to say "good thing that you don't do this because we really don't want to get into business of modeling return-by-value `getAs` calls such as `SVal::getAs`".



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:51
+
+  const CallDescriptionMap<CastCheck> SugarCastCDM = {
+      {{{"clang", "castAs"}, 0}, &CastValueChecker::evalCastAs},
----------------
Charusso wrote:
> NoQ wrote:
> > I'd rather have only one map from call descriptions to (callback, kind) pairs. This should make the `evalCall` code much more readable.
> The problem with that I really want to use something like that:
> ```
> auto &&[Check, Kind, MoreData] = *Lookup;
> ```
> but I cannot. Until that time it equally non-readable and difficult to scale sadly. For now it is fine, but that C++17 feature would be more awesome.
I think it turned out pretty pretty (no pun intended).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:26
 class CastValueChecker : public Checker<eval::Call> {
+  enum class CastKind { Checking, Sugar };
+
----------------
Please explain "Checking" and "Sugar". Checking what? Sugar... you mean syntactic sugar? For what?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:220
 
-  // If we cannot obtain both of the classes we cannot be sure how to model it.
-  if (!CE->getType()->getPointeeCXXRecordDecl() ||
-      !CE->getArg(0)->getType()->getPointeeCXXRecordDecl())
-    return false;
+  const CastCheck *Check = &Lookup->first;
+  CastKind Kind = Lookup->second;
----------------
What's the point of making this a raw pointer? (no pun intended)


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

https://reviews.llvm.org/D65889