clayborg added a comment. In https://reviews.llvm.org/D23884#526385, @zturner wrote:
> I'm not sure I follow. `StringRef` is literally just a wrapper around a > `const char*`, so if `const char*` works, why doesn't `StringRef`? "const char *" assumed null termination. StringRef can be a reference to part of a C string: const char *cstr = "\"valid json string\"and some other junk"; llvm::StringRef json_text(cstr, 19); Not that the last character after the ending quote isn't a '\0' charcter it is a 'a'. So if we want to actually use the "json_text" to parse the JSON, we need make a std::string so that it becomes null terminated. We could switch the parsing code to use a new class StringRefExtractor instead of StringExtractor in which case we can accept the data as a llvm::StringRef from the constructor. But all internal code would need to use std::string (GetToken() would still need to use std::string). > `JSONParser` constructor is already taking a `const char*` in this patch, so > it's not like it is modifying the contents of the string. You are correct. It does make a contract that used to not be there where the caller must guarantee the data it is parsing doesn't go away, but I don't think that will be a problem. > Checking the length and/or getting a substring or trimming values from the > beginning and end is going to be a far more common operation than converting > it into something that has to be null terminated (why does it have to be null > terminated for that matter anyway?) Currently it is so the parser knows when to stop, but it doesn't have to be if we switch to using a new StringRefExtractor class. > So the benefit of having the length stored with the string outweighs the con > of having to call `str()` when you want something null terminated, IMO Yes it it fine as long as you convert the parsing code. https://reviews.llvm.org/D23884 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
