https://github.com/bulbazord requested changes to this pull request.
+1 to all of Jim and Adrian's comments. High level comments: 1. I'm a little concerned about the use of assertions in this patch. I generally like assertions when they are used to assert internal consistency. It looks like you're using them as an error-checking and parameter-checking mechanism. This seems pretty fragile, especially since it will be difficult to enforce that these functions are called "correctly" in all cases from all contributors. 2. There isn't a lot of documentation. It would be helpful to know the expected behavior for a lot of these methods (especially for people like me who aren't super familiar with the ValueObject class and all of its tendrils). 3. Is there a way you can add some tests for the new functionality? I'm not sure how you could trigger these new methods from an API test, but a unit test might be possible? https://github.com/llvm/llvm-project/pull/87197 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
