vsavchenko marked 7 inline comments as done. vsavchenko added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:113 + +bool isObjCClassType(QualType Type) { + if (const auto *PointerType = dyn_cast<ObjCObjectPointerType>(Type)) { ---------------- NoQ wrote: > `static`? It is all inside of `anonymous` namespace, so no need in putting static here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:134 + // 1. Class is written directly in the message: + // \code + // [ActualClass classMethod]; ---------------- NoQ wrote: > 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? No, you are correct. It's more like a matter of habit and consistent look of the comment. I would anyway prefer to somehow tell the reader that this is a snippet. It can be with a more or less familiar way of doing it. (I've seen a good amount of doxygen-style comments for `static` functions in the project, I guess similar logic applies to those as well) ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:193 + // Types in Class objects can be ONLY Objective-C types + return {cast<ObjCObjectType>(DTI.getType())}; + } ---------------- NoQ wrote: > Why are we discarding `CanBeSubClassed` here? Good point, we should propagate it further ================ 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 ---------------- NoQ wrote: > 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. > I believe this is pretty much always the case. I didn't quite get what you mean by that ================ Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:919 + + ReceiverRuntimeType.Type->getSuperClassType(); + QualType ReceiverClassType(ReceiverRuntimeType.Type, 0); ---------------- NoQ wrote: > I don't think this line of code does anything. > > (time for some `LLVM_NODISCARD`???) Whoops, that's true! ================ 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 ---------------- NoQ wrote: > 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. We can re-do it in a separate commit I guess. ================ Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:1293 + if (Receiver == getSelfSVal().getAsRegion()) { return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl())); + } ---------------- NoQ wrote: > 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? Compiler knows it, but still marks it as an instance method (because technically `self` is an object). 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