[Lldb-commits] [PATCH] D30272: Improve data formatter for libstdcpp unique_ptr

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D30272#684610, @tberghammer wrote:

> My original plan was to add it to the synthetic child provider but after some thoughts and experimenting I concluded it doesn't really belongs to there because it should be responsible for generating new children while here we are modifying the way the parent object is printed.


That is true, however. It still seems that this property should be somehow tied to the child provider, because it is only in the presence of the provider that the unique_ptr obtains its magic dereferencing properties. For example, if I do a `frame variable -R X Y`, where `X` is a `std::unique_ptr` and `Y` is a variable that happens to have the same layout (but a different name) as `X`, I would expect them to be printed identically, but it seems this will break that.

> Also for this reason the implementation of it would be fairly complex because we would have to execute the synthetic child providers when the parent is referenced instead of when a child of it is queried. Also I think it won't be a good idea to add a new method to the synthetic child provider called "IsPointerLike" as it seems like introducing a very specific method to an interface what is currently quite generic. On the other hand I see the need for making the list of pointer like objects easily extensible from python.
> 
> One solution I can possibly see (haven't tried it out) is to add the following 2 new method to the value object:
> 
> - IsDeceremntPointerDepth: By default returns true for pointers and references and will return true for children generated for smart pointers
> - IsDereferenceOfParent: By default returns false and will return true for the first child of the smart pointers If we add these methods then the synthetic child provider can create a child where they will be returning true and then rely on them to implement this functionality. I think this solution would be a fairly clean design but would introduce a lot of new API for a small amount of extra functionality.

I like where this is going. I can see one possible problem here: the "IsDecrementPointerDepth"(*) property would now be a property of the child, whereas it really seems to be a property of the parent-child relationship. So we could have paths arriving at the same object and one will have this bit set and the other one will not. However this may not be a major problem as the ValueObjects seem to already know their parent anyway. I don't think the API blowup would be big. It seems you have had to add the `IsPointerLikeObject` method to quite a lot of classes for this implementation, and presumably these would not be needed if we go that way.

- I would try to keep "pointer depth" out of the name as this isn't something the values should need to worry about. Maybe something like IsObtainedByDereferencingAPointer, only shorter :)



================
Comment at: source/DataFormatters/ValueObjectPrinter.cpp:515
   const bool is_ptr = IsPtr();
+  const bool is_ref_or_ptr_like = IsRef() || IsPointerLikeObject();
   const bool is_uninit = IsUninitialized();
----------------
tberghammer wrote:
> labath wrote:
> > Is there are reason you are bundling the is-pointer-like with the is-ref flag instead of the is-ptr one (which would be more logical)? If there is one it certainly isn't obvious.
> Because I want to treat a unique_ptr the same way as a reference so if you just write so it is automatically dereferenced at the root level while not at other levels (we don't dereference pointers by default at any level). I can split it out to a separate bool if you think that helps
Splitting out would definitely help as that gives you space to explain why you chose to treat this differently. That said, I am not entirely sure this needs special treatment. If the smart pointer should behave like a pointer then maybe we short format it as such too (i.e., display the pointed-to value only if it was explicitly asked for).


https://reviews.llvm.org/D30272