vsavchenko marked 2 inline comments as done. vsavchenko added a comment. > Let's land this, I guess? Fantastic work!
Thanks :-) ================ 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: > vsavchenko wrote: > > NoQ wrote: > > > NoQ wrote: > > > > vsavchenko wrote: > > > > > NoQ wrote: > > > > > > vsavchenko wrote: > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > What i'm trying to say is that `C.getSVal(RecE)` and > > > > > > `Message.getSelfSVal()` and `Message.getReceiverSVal()` are > > > > > > basically the same `SVal`. It shouldn't be necessary to check both > > > > > > or check whether they're the same; you must have meant to check for > > > > > > something else, probably something purely syntactic. > > > > > > > > > > > > ---- > > > > > > > > > > > > > I inferred this by looking at > > > > > > > ObjCMethodCall::getInitialStackFrameContents(). > > > > > > > > > > > > Wait, so it's only true for inlined methods. For non-inlined > > > > > > methods `getSelfSVal()` will be unknown :/ > > > > > Yeah, that might be a bit extraneous to do it with `SVal`s, but this > > > > > code for sure does its job (it is definitely not a redundant check). > > > > > `getSelfSVal()` returns receiver of the function //containing// the > > > > > call and not the call itself. So, it does check if we the receiver of > > > > > the message is `self`. > > > > > > > > > > I changed it to this way of doing things because it is consistent > > > > > with how the same thing is done in `getRuntimeDefinition`. > > > > > `getSelfSVal()` returns receiver of the function containing the call > > > > > and not the call itself > > > > > > > > 😱 looks broken to me. > > > Let's rename `getSelfSVal()` so that it screamed that it's the callee's > > > self as opposed to the caller's self, and explain in a long comment why > > > do we even care about the caller's self. I.e., that we heuristically > > > assume that if a class method jumps into another class method on the same > > > class object, it's going to be devirtualized to the same class. Which > > > isn't always the case, hence !Precise. > > > > > > > > I don't really think that it's a good idea to mix these two worlds: > > > > - **world one** - interface function that allows to get an `SVal` for > > `self` of the containing method. It does exactly this, for no specific > > reason. I'm on board with renaming, but we need to come up with an > > appropriate name that describes what it gives and not why. > > - **world two** - use-case of this interface method that tries to figure > > out the type of the receiver (for devirtualization purposes or not). > > > > So, the comment can be only here. I agree, I can add more explanation about > > what we are doing in this particular piece of code, but it won't make any > > sense to add this (or similar) comment for `getSelfSVal()`. > Ideally i'd detach `getSelfSVal()` from `CallEvent` entirely. Like, it > doesn't even depend on the call site, why would it be part of `CallEvent` to > begin with? This additionally takes care of **world one**. > > > so that it screamed that it's the callee's self as opposed to the caller's > > self > > Mmm, the opposite, of course. `getCallerSelfSVal()`? > > Where do you think it should be? My vote goes to location context. ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:147 +static bool isLive(SymbolReaper &SR, const MemRegion *MR) { + return SR.isLiveRegion(MR); +} ---------------- NoQ wrote: > vsavchenko wrote: > > NoQ wrote: > > > Feel free to rename `isLiveRegion` instead if it helps :) > > Unfortunately, it is named a bit differently for a reason, - compiler > > couldn't decide which function to use `bool isLive(const MemRegion > > *region)` or `bool isLive(const VarRegion *VR, bool includeStoreBindings = > > false) const` for a call `isLive(VarRegion *)` (even though it's pretty > > obvious). Splitting `isLive` for `VarRegion` into two functions doesn't > > seem like an easy one. The only option I can see is the introduction of > > `isLiveImpl` (not very pretty in a header file) and two functions > > `isLive(const VarRegion*) const ` and `isLiveIncludingStoreBindings(const > > VarRegion *) const`. > > > Or rename `isLive(const VarRegion *, bool includeStoreBindings)`? (I suspect > this function should really be private anyway) I don't know about that, it doesn't seem like a good change. I think that all of them should be `isLive` 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