[Lldb-commits] [PATCH] D83450: Delegate UpdateChildrenPointerType to the Root ValueObject

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Assuming the goal is to make this code ValueObjectVariable-only, the code is fine. I can't say I understand why should it be ValueObjectVariable-only (*), but given how different the ValueObjectConstResult hierarchy is, I think that may be for the best. It would still be good to have a test case though, as the ValueObject hierarchy is in dire need of cleanup, and knowing all the constraints would definitely help.

(*) For example, I would have thought that removing this code from ValueObjectConstResult would make the test case added in D69273 <https://reviews.llvm.org/D69273> fail if "target variable" was replaced by the "expression" command. It turns out that is not the case -- expr works just fine. My guess is that this is because the ValueObjectConstResult classes are peppered with explicit `SetAddressTypeOfChildren(eAddressTypeLoad)` calls, which means the address never ends up being interpreted as a host address. It also explains why it was easy for that patch to flip an address type from "load" to "host", but it does not explain (to me, at least) why "host" would not be the right address type.

(**) E.g. the fact that we have both ValueObject{Cast,Child} and ValueObjectConstResult{Cast,Child} tells me that there is probably something wrong with the separation of concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83450