[PATCH] D78286: [analyzer] Track runtime types represented by Obj-C Class objects

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 6 inline comments as done.
vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:135
+  if (MsgE->getReceiverKind() == ObjCMessageExpr::Class) {
+    return {MsgE->getClassReceiver()->getAs<ObjCObjectType>(), true};
+  }
----------------
NoQ wrote:
> Let's explain what `true` means here, i.e. `/*Precise=*/true`.
Sure


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:163
+
+  // Check if the receiver is 'self'.
+  if (const DeclRefExpr *DRE =
----------------
NoQ wrote:
> I suspect that `self`, unlike `this`, can be reassigned in the middle of the method.
That's true, and we didn't account for that in here. I'll try to make a test on that (and maybe re-order how we handle these cases a bit


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:164-165
+  // Check if the receiver is 'self'.
+  if (const DeclRefExpr *DRE =
+          dyn_cast<DeclRefExpr>(RecE->IgnoreParenImpCasts())) {
+    const StackFrameContext *SFCtx = C.getStackFrame();
----------------
NoQ wrote:
> That's a good place for `const auto *`.
Right, didn't pay much attention to this moved code


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:217-218
 
 void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR,
                                               CheckerContext &C) const {
   ProgramStateRef State = removeDeadTypes(C.getState(), SR);
----------------
NoQ wrote:
> This needs to be filled in for the new trait as well! It's very important :3
Whoops! I forgot!
Good catch!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:324-325
+        // We used to assume that whatever type we got from inferring the
+        // type is actually precise (which is not exactly correct), and some
+        // the features of the analyzer depend directly on this.
+        //
----------------
NoQ wrote:
> Looks like a missed word somewhere.
OK, I'll try to re-write to make it more clear


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:37
 
+// A map from Class object symbols to the most likely contained type.
+REGISTER_MAP_WITH_PROGRAMSTATE(DynamicClassObjectMap, clang::ento::SymbolRef,
----------------
NoQ wrote:
> I'd rather word this as "pointed-to type".
Sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78286