tfiala added a comment.
Getting back to this change.
I'm going to rebase for code reformatting, remove the signature change on the
JSON parsing, adjust my call sites for it, refactor the structured data parsing
in the gdb-remote reception to a separate function, and then put this back up
for
tfiala added a comment.
In https://reviews.llvm.org/D23884#526419, @tfiala wrote:
> > Again, if we switch the parser over to use an llvm::StringRef then this
> > make sense, otherwise it doesn't.
>
>
> I don't want to conflate this change with switching the parser over to a
> string ref. The
tfiala added inline comments.
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:113
@@ -105,4 +112,3 @@
case 'J':
-// Asynchronous JSON packet, destined for a
-// StructuredDataPlugin.
{
+
zturner added a comment.
You mention a `StringRefExtractor`. But nothing about `StringExtractor` that I
can tell depends on the fact that it's null terminated. It only needs to know
where to stop. A `StringRef` contains a length, so you stop when it gets to
the end of the `StringRef`. It
clayborg added a comment.
In https://reviews.llvm.org/D23884#526390, @zturner wrote:
> Actually it looks like `JSONParser` constructor takes a `StringRef` and then
> converts it to a `std::string` internally. Why can't it just store the
> `StringRef` internally? It doesn't modify the buffer,
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
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Actually after looking at the JSON parser I see that it isn't using
llvm::StringRef throughout. So I vote to keep the "const char *" for now since
we need to guarantee that the JSON is
tfiala added a comment.
Greg's going to change over the parser now to StringRef. I'll adjust my patch
once he does that.
https://reviews.llvm.org/D23884
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
We should probably move over to using llvm::StrringRef instead of "const char
*". It the JSON parser isn't using llvm::StringRef already it probably should.
zturner added a comment.
Actually it looks like `JSONParser` constructor takes a `StringRef` and then
converts it to a `std::string` internally. Why can't it just store the
`StringRef` internally? It doesn't modify the buffer, and incidentally it also
keeps a `uint64_t` representing the
zturner added a subscriber: zturner.
zturner added a comment.
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`? `JSONParser`
constructor is already taking a `const char*` in this patch, so it's not like
clayborg added a comment.
Looking closer at the JSON parser llvm::StringRef isn't the right thing to use
when parsing JSON. The parser will often remove desensitizing characters from
say a string like:
"hello \"world\""
And when parsing a token in JSONParser::GetToken, we can't just hand
labath added a comment.
Thank you for writing the tests. I have two stylistic comments, but otherwise
looks great.
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:113
@@ -105,4 +112,3 @@
case 'J':
-// Asynchronous JSON packet,
tfiala created this revision.
tfiala added reviewers: clayborg, labath, jasonmolenda.
tfiala added a subscriber: lldb-commits.
This change does the following:
* Adds three new unit test entries to test handling of $JSON-asyc: packets.
Thanks to Pavel for making that whole section of code easily
14 matches
Mail list logo
