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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to