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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

So IIUC we can do things here:

1. We disallow giving `ValueObjectConstResult` a new value via `SetData`/`SetValueFromCString`. Both functions have a way of signaling error to the user, so that could work. I do wonder how many users right now try to reassign a `ValueObjectConstResult` a new value and expect that to work (or do the error handling).
2. This patch which fixes the side-effects of giving `ValueObjectConstResult` a new value (which we already allow in LLDB).

I think this solution here is fine as it seems reasonable thing to do from a user's perspective (which don't know that they actually have a `ConstResult` underneath). Also given that `ValueObjectConstResult` is more like a "`ValueObjectHostCopy`" this doesn't seem too outlandish of a feature. And it doesn't break any existing code.

I'll accept this but please wait until Jim had a chance to have a look (just because having another pair of eyes on ValueObject patches seems like a good idea)



================
Comment at: lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py:140
+
+        b.SetValueFromCString(frame0.FindVariable("b2").GetValue())
+        self.assertEquals(
----------------
Can you assert that this returns `True`?


================
Comment at: lldb/test/API/python_api/value/change_values/main.c:28
+  b1->value = 1;
+  struct bar *b2 = (struct bar *) malloc (sizeof (struct bar));
+  b2->value = 2;
----------------
Could this just point to local/global `struct bar` variables instead? I would prefer not adding more references to external code (`malloc`) in this test if possible and it would prevent the memory leak in this test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470