clayborg added a comment. 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 main issue is people are already using this API this way. We can fix this by adding documentation and saying "don't do that", but that will lead to bugs and our API appearing flaky. Many strings we return have no lifetime issues due to string constification. We can't really even stop people from doing this by making sure the string is emptied when the object destructs because another object could come and live in its place quickly on the stack and we could be using a bogus data as the error string. All that said, I am fine going with what the majority of people want on this. I will remove this issue from this patch so it doesn't hold up this patch. 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
