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

Added input buffer offsets to DOM objects and a line offset buffer. #39

Closed
wants to merge 1 commit into from
Closed

Conversation

alexshafranov
Copy link

@alexshafranov alexshafranov commented Jun 25, 2016

As discussed in #38 I'm adding line information to DOM objects.

  • json_value_s stores a byte offset of the value
  • json_object_element_s stores a byte offset of the element name
  • line offset table is optionally exposed via json_parse_result_s
  • offsets can be translated to locations using binary search client-side, e.g. by using std::lower_bound

@sheredom
Copy link
Owner

Thanks for this! I'm going to show this PR to some of the other users of the lib and make sure they are ok with it (I assume they will be of course). Give me a few days or so for them to comment 😄

Awesome work!

@alexshafranov
Copy link
Author

Sure! Thanks for looking at the code

@sheredom
Copy link
Owner

So I've had some feedback that for some of the users that this will effect their memory budgets too highly.

But, I've actually had an idea of how to implement what you want, while retaining that the 'default' approach doesn't contain this data. TL;DR I'm working on an alternative now watch this space! 😄

@alexshafranov
Copy link
Author

alexshafranov commented Jun 29, 2016

Right, good to know you have an alternative plan in mind :)

Out of curiosity, are you maybe going to store json_value_s contiguous in the allocated buffer, so that offsets can be accessed by an index?

@sheredom
Copy link
Owner

That would be a much bigger change than I had in mind.

My current thought was just to have another struct json_value_ex_s - that contained the offset + line_no + row_no. Then have a parsing option for json_parse_ex that replaces all json_value_s' with json_value_ex_s' instead, and that way you would just need to cast the json_value_s -> json_value_ex_s to get the location information of any value.

Would that be sufficient for your use-case?

@alexshafranov
Copy link
Author

alexshafranov commented Jun 29, 2016

Cool, that will work for my use case, except that I also added offset information to object keys in json_object_element_s.
Not sure how to handle that at the moment

@sheredom
Copy link
Owner

Hmm. I'm guessing it is really useful to you to know where the key exactly is because that allows you to error when the key doesn't match your schema in some way?

@alexshafranov
Copy link
Author

alexshafranov commented Jun 29, 2016

Yes, that's how I wanted to use it.

But now I'm actually thinking that key offset is probably the only thing I need.
Like if the value isn't of the expected format, then the corresponding key location is reported

@alexshafranov
Copy link
Author

So what if we just remove an offset field from json_value_s and keep the one in json_object_element_s? Would that be suitable for memory concerned users?

@sheredom
Copy link
Owner

They basically don't want any extra memory for their normal use cases, that's why I've done #40 where you just cast the value -> value_ex, and the object name string -> string_ex to get what you need! 😄

@alexshafranov
Copy link
Author

Awesome, *_ex approach should work for me!

@sheredom
Copy link
Owner

#40 is now merged, thanks for this inspirational MR though! Really helped me understand what you needed 😄

@sheredom sheredom closed this Jun 30, 2016
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.

None yet

2 participants