Fix empty commented arrays in legacy Reader - #1719
vaibhav0806 wants to merge 1 commit into
Conversation
|
| if (!readToken(comment)) { | ||
| return addErrorAndRecover( | ||
| "Syntax error: value, object or array expected.", comment, | ||
| tokenArrayEnd); | ||
| } | ||
| skipSpaces(); |
There was a problem hiding this comment.
Comments attach to sibling values
With comment collection enabled, {"a":[ /* inner */],"b":1} now parses as an empty array, but the comment remains pending when that array closes. Reading b then attaches the comment to b instead of the array that contains it, so the parsed comment is assigned to the wrong value.
There was a problem hiding this comment.
Thanks for flagging this; the reported placement reproduces. On original master 3347a4b, CharReader already assigns /* inner */ to b for this exact input. Legacy Reader does the same for the equivalent empty-object control, {"a":{\n/* inner */},"b":1}. Legacy Reader rejects the empty-array example and leaves a partial a:[null], so there was no successful legacy array result to preserve.
A local C++11 comparison covered 14 documents × collection on/off × both APIs (56 observations per library), checking parsed data and every node's comment placements. All 28 modern-reader observations and all 14 previously successful legacy controls were unchanged; all patched legacy results matched original-master CharReader data/comments. Cases included the exact example, line/block comments, last members, nested arrays, preceding siblings, and nonempty-array/empty-object controls. This supports compatibility with existing behavior, while the concern about comment ownership remains valid. Moving inner comments onto empty containers would change existing semantics; this PR keeps the parsing fix narrow.
There was a problem hiding this comment.
Thanks for validating this against both the original legacy behavior and CharReader. I agree the comment placement is existing semantics rather than a regression introduced by this change: the same input already assigns the comment to the following sibling in CharReader, and moving it onto the empty array would make legacy behavior diverge from both the established modern behavior and the equivalent empty-object case. Given that the comparison shows no changes to previously successful controls or modern parsing, I’m withdrawing this finding; no change is needed.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Fixes #1238.
With comments enabled, legacy
Json::Readerrejects{"list":[ //\n]}and leaves a partial array containing null.CharReaderalready accepts the same document as an empty array.Consume allowed line/block comment tokens before the legacy reader's initial empty-array delimiter check, so it does not create element zero before discovering
]. Use the existing error recovery for malformed comments. Add four regression fixtures covering root/nested arrays, collection on/off, comments disallowed, unterminated comments, ordinary element/container comments, and offsets.Only
Reader::readArrayand its tests change. Public headers, declarations and class layout are unchanged. Modern parsing, numeric conversion, trailing-comma handling and comment storage are unchanged.Local validation on macOS arm64, Apple Clang 17, C++11, Debug, default configuration and static library:
git diff --checkand clang-format 17/20 checks pass; both formatters produce identical output for the changed files.The seven upstream workflows currently await maintainer approval on head
167330d624e203b35d4df35a33b935f04f810bea; no jobs or checks have run. Exact clang-format 18, Linux/Windows/shared-library builds and other language standards remain unverified. cppcheck was unavailable locally, so no static-analysis success is claimed.Prepared with AI assistance. The generated diff was independently checked and the local validation was run by an automated agent. No human review is claimed.