labath added a comment. The vscode change seems fine to me, but I don't consider myself an authority on that.
In D134333#3812653 <https://reviews.llvm.org/D134333#3812653>, @jingham wrote: > In D134333#3812448 <https://reviews.llvm.org/D134333#3812448>, @clayborg > wrote: > >> I just did a search through our test sources and we use >> GetError().GetCString() 34 times in our test suites python files. So the >> SBError::GetCString() is being misused a lot like this already and is >> probably making some tests flaky. I would highly recommend we fix >> SBError::GetCString() to make sure things work correctly. If people are >> already doing this, or if they can do this with our API, then we should make >> our API as stable as possible for users even if it costs a bit more memory >> in our string pools. > > When we return Python strings from the SWIG'ed version of > GetError().GetCString() does the Python string we make & return copy the > string data, or is it just holding onto the pointer? In C/C++ if someone > returns you a char * unless explicitly specified you assume that string is > only going to live as long as the object that handed it out. But a python > string is generally self-contained, so you wouldn't expect it to lose its > data when the generating object goes away. I haven't checked yet, but if > this is how SWIG handles making python strings from C strings, then this > wouldn't be an error in the testsuite, only in C++ code. If that's not how > it works, that might be a good way to finesse the issue. The python version is fine because swig SBError::GetCString wrapper gets essentially a pointer to the SBError objects (wrapped as PyObject), and returns another PyObject with the (copied) string (also a PyObject). All the lifetime management happens outside of that function. ================ Comment at: lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py:48-53 + contains = verify_value in actual_value + self.assertTrue(contains, + ('"%s" value "%s" doesn\'t contain' + ' "%s")') % ( + key, actual_value, + verify_value)) ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134333/new/ https://reviews.llvm.org/D134333 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
