Skip to content

Conversation

@redboltz
Copy link
Contributor

@redboltz redboltz commented May 1, 2016

The current unpacking APIs are constructed on the visitor mechanism.
(Fixed #418)

The current unpacking APIs are constructed on the visitor mechanism.
(Fixed msgpack#418)

Updated test condition.
@redboltz
Copy link
Contributor Author

redboltz commented May 2, 2016

There is no significant difference in performance.
image


#if MSGPACK_DEFAULT_API_VERSION >= 2

struct json_visitor : msgpack::v2::null_visitor {
Copy link
Contributor

@jpetso jpetso May 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this test is also listed on the wiki documentation page, I'll point out two potential issues:

  • end_map_value() doesn't append a comma to separate map items like end_array_item() does.
  • visit_str() doesn't process strings as Unicode, therefore non-ASCII codepoints will result in invalid JSON (which encodes them as e.g. "\u00f8").

Neither of these is required by the test case so that's okay for this particular piece of code, however you might want to point it out in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comments.

end_map_value() doesn't append a comma to separate map items like end_array_item() does.

I will fix it.

visit_str() doesn't process strings as Unicode, therefore non-ASCII codepoints will result in invalid JSON (which encodes them as e.g. "\u00f8").

True. The point of the test and the document is not accurate JSON, I just demonstrate a visitor usage. However, I should use a precise term. I will fix the test and the document. I will use the term 'json like structure ( string is not espaced)'.

@jpetso
Copy link
Contributor

jpetso commented May 2, 2016

One more thing to consider: When I was having a look at https://github.com/msgpack/msgpack-c/wiki/v2_0_cpp_unpack_visit, I realized that this code isn't actually unpacking the msgpack buffer, at least not in the traditional sense of unpack as "parse buffer into msgpack::object".

Since the function name (unpack_visit()) is already not the same as the traditional unpack(), I wonder whether it makes sense to give it a name that's less tied to the traditional meaning of "unpack". Maybe parse()? I'm interested to hear about your thoughts on this idea :)

@redboltz
Copy link
Contributor Author

redboltz commented May 3, 2016

Good point. unpack_visit() doesn't unpack anything. I think that parse() is good too. Another candidate is traverse(). In this context, parse() is better. I will rename to parse().

@redboltz
Copy link
Contributor Author

redboltz commented May 5, 2016

I've fixed all problems that you pointed out. In addition, I found end_map_value() calling point was wrong, and fixed it. https://github.com/redboltz/msgpack-c/blob/add_unpack_visitor_api/include/msgpack/v2/unpack.hpp#L434

Renamed names that related to a visitor.
Renamed test name from json to json_like because I omit escape.
Added end_map_value() and end_map() implementation.
@redboltz redboltz force-pushed the add_unpack_visitor_api branch from 44d2dfc to 6593cba Compare May 5, 2016 09:45
@redboltz
Copy link
Contributor Author

redboltz commented May 9, 2016

@jpetso , could you review my fix? If my fix would be ok, I will merge it.

@redboltz
Copy link
Contributor Author

I am going to merge the PR this weekend if no comments are post.

@redboltz redboltz merged commit d15d922 into msgpack:master May 22, 2016
@redboltz redboltz deleted the add_unpack_visitor_api branch May 22, 2016 13:50
@jpetso
Copy link
Contributor

jpetso commented May 24, 2016

Hi, sorry, and well done! I had a pretty tight few weeks and didn't manage to find the time for the review. Since the first version of the patch was already very good, I'm assuming it's quite alright :)

Anyway, I'm glad it's in, and thank you for your hard work!

@redboltz
Copy link
Contributor Author

@jpetso , thank you!

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