[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This change is wrong for ValueObjectConstResult's.  The original point of ValueObjectConstResult's was to store the results of expressions so that, even if the process is not at the original stop point, you could still check the value.  It should have the value it had at the time the expression was evaluated.  Updating the value is exactly what you don't want to do.  So I think you need to make sure that we can't change their value after they are created.

Note, ExpressionResult variables are different from ExpressionPersistentVariables.  The former should not be updated, and after the process moves on any children that weren't gathered become "unknown".  But ExpressionPersistentVariables should be able to be assigned to, and when the reference target object, it should update them live.  I think this is a useful distinction, but it's somewhat messily expressed in the current state of this code.  That should get cleaned up IMO but in the meantime we shouldn't make it muddier...

The other thing to check about this patch is whether it defeats detecting changed values in child elements.  If I make a ValueObject that tracks a value in the target - i.e. ValueObjectVariable - and fetch some of its children, then step, I should be able to call ValueObject::GetValueDidChange on the child element and get a correct result.  It seems to me that if you throw away the children at the beginning of the update I don't see how you would have anything to compare to.  There are a few tests for GetValueDidChange (python_api/value_var_update/TestValueVarUpdate.py).  It would be good to first understand whether they just didn't test thoroughly, or if this IS still working, how.  It wouldn't be good if this continued working by some accident that was going to break later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470