-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stage 3 Specification Review: Waldemar Horwat #29
Comments
Here's my review. I just have a couple editorial comments. The ShallowestContained algorithm doesn't work in general. It may happen to work for the JSON subset, but it forms an attractive nuisance that hides land mines for others who might call it in the future. For example, ShallowestContained called on the parse of "class C {x=4; [true]=8}" with types «NumericLiteral» doesn't find any numeric literals. But if we add searching for an unrelated nonterminal, it suddenly starts to find them: ArrayElementList is a bit broken in general too. If elisions are present, the length of the list generated by ArrayElementList is not necessarily equal to the array's length because a single Elision can contain multiple commas but ArrayElementList treats it as a single element. However, the JSON syntax doesn't allow elisions, so it happens to work on JSON arrays. Possible solutions are to either fix these so they work in general or rename them so it's clear they are only usable on JSON objects. |
Responding to [#29 (comment)](#29 (comment)) > _ArrayElementList_ is a bit broken in general too. If elisions are present, the length of the list generated by _ArrayElementList_ is not necessarily equal to the array's length because a single _Elision_ can contain multiple commas but _ArrayElementList_ treats it as a single element. However, the JSON syntax doesn't allow elisions, so it happens to work on JSON arrays.
Thank you, this is an excellent point. Please review #30, which renames the operation to ShallowestContainedJSONValue and narrows its use to
This behavior is intentional, although the length assertion is indeed specific to |
This is a placeholder task for the Stage 3 Specification Review feedback from @waldemarhorwat.
The text was updated successfully, but these errors were encountered: