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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
