Skip to content
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

track JsonFactory in CharsToNameCanonicalizer #1086

Closed

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Aug 23, 2023

  • not yet making a change for ByteQuadsCanonicalizer
  • based on validate the length of names #1078 (comment)
  • I don't really understand the subtleties in CharsToNameCanonicalizer. In validate the length of names #1078, I am already validating the length before I call findSymbol on the CharsToNameCanonicalizer so I don't know what benefit there is to also checking in CharsToNameCanonicalizer. The changes in ReaderBasedJsonParser in 1078 are incomplete because waiting until the CharsToNameCanonicalizer findSymbol call is very late. We may already have wasted a lot of processing time by that stage.

@cowtowncoder
Copy link
Member

Point is not to have check in both places, but only in CharsToNameCanonicalizer. For performance reasons check for max length really should not/can not be in tight loop for decoding, unfortunately; things are bit easier for byte-backed case.

But FWTW I am mostly/only worried about byte-based side, decoding of quad-based input into String: not for collecting quads themselves (or char buffer). These can be detected right before processing collected raw input.
Some users might prefer more exact failure for exact limit, but that is very difficult to implement for byte-based input (at least wrt limit being defined as chars, if that was supported).

@cowtowncoder
Copy link
Member

@pjfanning I implemented this slightly differently, see #1088. Closing this PR as not needed any more.

@pjfanning pjfanning deleted the CharsToNameCanonicalizer-factory branch August 24, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants