NoQ added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:113 + +bool isObjCClassType(QualType Type) { + if (const auto *PointerType = dyn_cast<ObjCObjectPointerType>(Type)) { ---------------- `static`? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:134 + // 1. Class is written directly in the message: + // \code + // [ActualClass classMethod]; ---------------- These comments will never be parsed by doxygen, i don't think those `\code` thingies are needed here. Or is it some editor/IDE-specific thingies that i'm not educated about? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:193 + // Types in Class objects can be ONLY Objective-C types + return {cast<ObjCObjectType>(DTI.getType())}; + } ---------------- Why are we discarding `CanBeSubClassed` here? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:198 + // 'self' variable of the current class method. + if (ReceiverSVal == Message.getSelfSVal()) { + // In this case, we should return the type of the enclosing class ---------------- I believe this is pretty much always the case. At least whenever `getInstanceReceiver()` is available. Another exception seem to be when `ReceiverSVal` is an `UnknownVal` (in this case `self` is going to be `SymbolRegionValue` because it's never set in the Store), but that's it. I inferred this by looking at `ObjCMethodCall::getInitialStackFrameContents()`. I think we should have used `getSelfSVal()` to begin with. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:919 + + ReceiverRuntimeType.Type->getSuperClassType(); + QualType ReceiverClassType(ReceiverRuntimeType.Type, 0); ---------------- I don't think this line of code does anything. (time for some `LLVM_NODISCARD`???) ================ Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:1219-1226 + // NOTE: This cache is essentially a "global" variable, but it + // only gets lazily created when we get here. The value of the + // cache probably comes from it being global across ExprEngines, + // where the same queries may get issued. If we are worried about + // concurrency, or possibly loading/unloading ASTs, etc., we may + // need to revisit this someday. In terms of memory, this table + // stays around until clang quits, which also may be bad if we ---------------- Before i forget: Stuff that's shared across analyses usually lives in `AnalysisConsumer`. Cf. `FunctionSummaries`. This is the intended way of avoiding globals in such cases. ================ Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:1293 + if (Receiver == getSelfSVal().getAsRegion()) { return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl())); + } ---------------- Wait, how do we get a decl here that's anyhow relevant if the compiler doesn't even know that it's a class method? ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:85-87 + if (const DynamicTypeInfo *DTI = State->get<DynamicClassObjectMap>(Sym)) + return *DTI; + return {}; ---------------- My favorite idiom for this stuff so far: ```lang=c++ const DynamicTypeInfo *DTI = State->get<DynamicClassObjectMap>(Sym); return DTI ? *DTI : {}; ``` (not sure `?:` will typecheck correctly in case of `{}` though) ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:147 +static bool isLive(SymbolReaper &SR, const MemRegion *MR) { + return SR.isLiveRegion(MR); +} ---------------- Feel free to rename `isLiveRegion` instead if it helps :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78286/new/ https://reviews.llvm.org/D78286 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits