[PATCH] D66325: [analyzer] CastValueChecker: Store the dynamic types and casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicCastInfo.h:19
+public:
+  enum CastKind { Success, Fail };
+
----------------
I suggest `enum CastResult { Success, Failure }` ("a fail" is slang, also "result" because it's basically a result).

Also TODO: The success information should ideally go straight into the dynamic type map.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicCastInfo.h:31-32
+
+  bool isSucceeds() const { return Kind == CastKind::Success; }
+  bool isFails() const { return Kind == CastKind::Fail; }
+
----------------
`succeeds()`, `fails()` (valid English).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h:28
 /// symbol to its most likely type.
-struct DynamicTypeMap {};
+REGISTER_MAP_WITH_PROGRAMSTATE(DynamicTypeMap, const clang::ento::MemRegion *,
+                               clang::ento::DynamicTypeInfo)
----------------
Can we move these macros into the cpp file so that they were only accessed by the fancy accessors?

Also i don't remember whether these macros do actually work correctly in headers. The original code was doing the trait manually because it had `GDMIndex()` defined in the cpp file, but if we put these macros into the header we'll have a static variable defined in multiple translation units, which may cause it to have different addresses in different dynamically loaded libraries that include this header.

You might need to merge your `checkDeadSymbols()` into the existing `checkDeadSymbols()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:83-93
+static QualType getRecordType(QualType Ty) {
+  Ty = Ty.getCanonicalType();
+
+  if (Ty->isPointerType())
+    return getRecordType(Ty->getPointeeType());
 
+  if (Ty->isReferenceType())
----------------
Most of the time we should know exactly how many pointer/reference types we need to unwrap. I really prefer we hard-assert this knowledge instead of blindly unwrapping as many pointer/reference types as we want. Because every time we have an unexpected number of unwraps it's an indication that something went horribly wrong. So it's good to have the extra sanity checking.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:115
+
+  const DynamicCastInfo *Cast = getDynamicCastInfo(State, CastFromTy, CastToTy);
+
----------------
It would have been much easier for me to read if this variable was called `CastInfo`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:118
+  // We assume that every checked cast succeeds.
+  bool IsCastSucceeds;
+  if (Cast)
----------------
Just `CastSucceeds` (valid English).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:145-152
+        if (!IsCheckedCast)
+          Out << (IsKnownCast ? "Dynamic cast" : "Assuming dynamic cast");
+        else
+          Out << "Checked cast";
 
-        Out << "to '" << CastToName << "' "
-            << (!IsNullReturn ? "succeeds" : "fails");
+        Out << " from '" << CastFromTy->getAsCXXRecordDecl()->getNameAsString()
+            << "' to '" << CastToTy->getAsCXXRecordDecl()->getNameAsString()
----------------
To think: in D66423 you added this fancy feature when you dump the current dynamic type of the object instead of the static type. I think this is super cool and we should do it here as well, because the dynamic type is pretty much the only thing that you won't be able to figure out by looking at the source.

We could give the same message as for `isa`, say, `Assuming the object is a 'Circle'` etc.


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicTypeMap.cpp:83-86
+  CastSet Set = F.getEmptySet();
+
+  if (const CastSet *TempSet = State->get<DynamicCastMap>(MR))
+    Set = *TempSet;
----------------
My favorite way of writing this stuff:
```lang=c++
const CastSet *SetPtr = State->get<DynamicCastMap>(MR);
CastSet Set = SetPtr ? *SetPTr : F.getEmptySet();
```


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

https://reviews.llvm.org/D66325