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

Fix iterator_input_adapter consuming more than it should #3531

Conversation

falbrechtskirchinger
Copy link
Contributor

As identified by @imwhocodes, iterator_input_adapter consumes an additional character from the input during parsing. This PR based on #3442 by @imwhocodes fixes this. I'm finishing his work by incorporating requested changes and fixing CI issues.

At the moment, it's missing a unit test.

Replaces #3442.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling dc809d4 on falbrechtskirchinger:iterator_input_adapter_fix into 48a102c on nlohmann:develop.

@falbrechtskirchinger falbrechtskirchinger marked this pull request as draft June 12, 2022 08:02
@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Jun 12, 2022

I've tried to write a unit test and the "fixed" iterator_input_adapter isn't consuming enough. Either my test is flawed or there was never a problem to begin with. 😕

Parsing "[1][2]":

TEST CASE:  deserialization
  contiguous containers
  via iterator range
  verify iterator_input_adapter doesn't over-consume

/home/flo/projects/json/iterator_input_adapter_fix/tests/src/unit-deserialization.cpp:618: ERROR: CHECK( std::string(first, last) == str2 ) is NOT correct!
  values: CHECK( ][2] == [2] )

@falbrechtskirchinger
Copy link
Contributor Author

I've now added the unit test which fails to trigger the alleged issue.

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Jun 23, 2022

I'm convinced this fixes nothing and breaks legitimate use cases. See #3442 for a lengthy discussion.

@falbrechtskirchinger falbrechtskirchinger deleted the iterator_input_adapter_fix branch July 25, 2022 05:17
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