Added support for key types in msgpack: added sax parser key types: null, bool, unsigned, integer, float

[imwhocodes]

Hi

This pull request add support in the sax parser and in the msgpack deserializer for object key of types:

• null
• boolean
• unsigned
• integer
• float

As described in in issue this change is not breaking the API and behaviour

This PR consist of 2 part:

1. adding the necessary code to get_msgpack_object
2. refactoring the interface and inheritance model

As you can see now there is a class sax_interface that define the common interface (only pure virtual member functions) for all sax like classes json_sax now inherit from sax_interface, json_sax simply override the newly added functions to return false keeping the API compatible to before this commit

Part of the refactoring was also moving the sax like testing classes:

• SaxEventLogger
• SaxCountdown

to a common place, and also make them inherit from sax_interface (before they were not inheriting from anything) Also some code was abstracted in common json_sax_exception_handler

If you have any doubt please ask, Luca

---

Pull request checklist

• [Y] Changes are described in the pull request, or an existing issue is referenced
• [Y] The test suite compiles and runs without error.
• [?] Code coverage is 100%. Test cases can be added by editing the test suite.
• [Y] The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

[falbrechtskirchinger]

A few issues at first glance:

• The internal SAX classes should continue to avoid polymorphic inheritance.
• Existing classes should still work without having to add the new member functions.
• You forgot to update detail::meta::is_sax. Any update would also need to handle the previous point.
• (More as a reminder to myself.) I'd like to better understand if there's a noteworthy performance impact to the parser for users not using non-string keys. Looks like some duplicated tests might be occurring.

I'll go over your code in more detail later. Those are just some quick first thoughts.

[nlohmann]

I do not like the changes as they are proposed right now. I understand our MsgPack implementation lacks support for non-string object keys, but this change has an effect to a way larger part of the code.

[imwhocodes]

For sure this PR is not meant to be merged as it is without a proper review

My thoughts on your point @falbrechtskirchinger:

1. Yes It it possible to avoid polymorphic inheritance in the internal SAX classes, but is not clear to me what are the advantage to the absence of proper and well defined polymorphism, I get that the functions using sax are templated on the sax itself and there is no need for the compilation, but logically that is the common interface and it should be inherited (also in the original code for SaxEventLogger and SaxCountdown was implemented multiple times, some of it inheriting other without, now everything is common)
2. Existing classes should still work without having to add the new member functions <- user implemented sax parse work exactly as before, of course internal classes should adapt to inner working of the library
3. Yes I forgot that (detail::meta::is_sax)
4. Conceptually between compiler devirtualisation and optimization the final codepath should be almost (if not exactly) the same in the case of no overload of the added functions

@nlohmann Is there any specific thing that I can address?

Thanks, Luca

[falbrechtskirchinger]

For sure this PR is not meant to be merged as it is without a proper review

My thoughts on your point @falbrechtskirchinger:

1. Yes It it possible to avoid polymorphic inheritance in the internal SAX classes, but is not clear to me what are the advantage to the absence of proper and well defined polymorphism, I get that the functions using sax are templated on the sax itself and there is no need for the compilation, but logically that is the common interface and it should be inherited (also in the original code for SaxEventLogger and SaxCountdown was implemented multiple times, some of it inheriting other without, now everything is common)

I'd generally stay away from dynamic polymorphism if the same outcome can be achieved without it (and the alternative isn't prohibitively complex).

I'd also not pose those same restrictions on test code. Just on the library.

2. Existing classes should still work without having to add the new member functions <- user implemented sax parse work exactly as before, of course internal classes should adapt to inner working of the library

Indeed. I see you haven't modified any library code using the SAX interface.

3. Yes I forgot that (detail::meta::is_sax)

(I meant detail::is_sax in detail/meta/is_sax.hpp.)

4. Conceptually between compiler devirtualisation and optimization the final codepath should be almost (if not exactly) the same in the case of no overload of the added functions

I haven't looked closely enough but don't you have (some of) the same switch cases in get_msgpack_object and get_msgpack_string (and maybe other get_msgpack_*() functions)?

I'm working on ~two~ four PRs right now myself and am considering a ~third~ fifth. Again, those were just my first thoughts and I'll do a proper review some other time. Maybe next week.

[deepbluev7]

How about instead of making the key functions pure virtual, giving them a default implementation in the existing interface and doing the customization in some derived msgpack/cbor/whatever parser? Then internally it wouldn't use any new inheritance and the integer key part is currently an error anyway, so some slowdown in those cases shouldn't be relevant, but it means users of this lib can at least access integer key parsing if they need it.

[imwhocodes]

How about instead of making the key functions pure virtual, giving them a default implementation in the existing interface and doing the customization in some derived msgpack/cbor/whatever parser? Then internally it wouldn't use any new inheritance and the integer key part is currently an error anyway, so some slowdown in those cases shouldn't be relevant, but it means users of this lib can at least access integer key parsing if they need it.

class json_sax already have the implementation of the new functions defaulted to return false It would be better to throw an exception or signalling in some way that it is not supported

[github-actions[bot]]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated.

[imwhocodes]

Following the style of the rest of the library I moved the implementation from polymorphism to template/typetraits

Now the binary_reader dispatch at compilation time based on the traits of the sax_parser on which is templated

This permit to extend the parser without breaking code based on old interface (that is de-facto the same)

This is not meant to be the final commit, I'm asking for feedback before adding the new needed test

[github-actions[bot]]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated.