Copilot commented on code in PR #3635:
URL: https://github.com/apache/brpc/pull/3635#discussion_r4334192993
##########
src/butil/third_party/rapidjson/reader.h:
##########
@@ -1539,10 +1539,9 @@ class GenericReader {
if ((parseFlags & kParseStopWhenDoneFlag) && state ==
IterativeParsingFinishState) {
// wwb: Update parseResult_.Offset() when
kParseStopWhenDoneFlag
// is set which means the user needs to know where to resume
- // parsing in next calls to JsonToProtoMessage()
- if (is.Peek() != '\0') {
- SetParseError(kParseErrorNone, is.Tell());
- }
+ // parsing in next calls to JsonToProtoMessage(), including
+ // when the root ends exactly at EOF.
+ SetParseError(kParseErrorNone, is.Tell());
Review Comment:
This is a vendored third-party header, but the comment is project-specific
(`JsonToProtoMessage()`) and includes an author tag (`wwb:`). To keep the
third-party code easier to maintain/rebase, reword the comment to be
library-generic (e.g., `caller needs resume offset for iterative parsing`) and
drop personal/project-specific references.
##########
src/butil/third_party/rapidjson/reader.h:
##########
@@ -1539,10 +1539,9 @@ class GenericReader {
if ((parseFlags & kParseStopWhenDoneFlag) && state ==
IterativeParsingFinishState) {
// wwb: Update parseResult_.Offset() when
kParseStopWhenDoneFlag
// is set which means the user needs to know where to resume
- // parsing in next calls to JsonToProtoMessage()
- if (is.Peek() != '\0') {
- SetParseError(kParseErrorNone, is.Tell());
- }
+ // parsing in next calls to JsonToProtoMessage(), including
+ // when the root ends exactly at EOF.
+ SetParseError(kParseErrorNone, is.Tell());
Review Comment:
Using `SetParseError(kParseErrorNone, ...)` to update a successful parse
offset is non-obvious because the API name implies error handling. If RapidJSON
exposes a clearer way to set the `ParseResult` offset on success (e.g.,
assigning `parseResult_` / calling a `Set`-style method), prefer that;
otherwise, consider adding a brief clarifying comment explaining that this is
the intended way to update `parseResult_.Offset()` for successful parses.
##########
test/brpc_protobuf_json_unittest.cpp:
##########
@@ -1689,6 +1690,101 @@ TEST_F(ProtobufJsonTest, string_to_int64) {
ASSERT_EQ(person.data(), 1234567);
}
+TEST_F(ProtobufJsonTest, parse_json_offset_at_eof) {
+ const struct {
+ const char* json;
+ size_t expected_offset;
+ bool array_to_single_repeated;
+ } cases[] = {
+ {"{}", 2, false},
+ {" \t{}", 4, false},
+ {R"({"person":[]})", 13, false},
+ {"[]", 2, true},
+ {"{} \n", 2, false},
+ {"{}{}", 2, false},
+ };
Review Comment:
The test name `parse_json_offset_at_eof` is misleading because the cases
include non-EOF-stop scenarios (e.g., `\"{}{}\"` where the first value ends
before EOF, and `\"{} \\n\"` where the stream is not at EOF when the JSON root
ends). Consider renaming the test to reflect
`kParseStopWhenDoneFlag`/\"stop-when-done\" offset behavior, or split cases so
the `*_at_eof` test only covers true EOF-end inputs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
