[Lldb-commits] [PATCH] D23884: Add StructuredData unit tests; remove JSON parsing string copy

[Greg Clayton via lldb-commits]

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