Feature/user defined types #338

theodelrieu · 2016-10-19T12:11:15Z

WIP: This PR is NOT ready to be merged, its purpose is to facilitate review/discussions.

Files to change

There are currently two files which need to be edited:

src/json.hpp.re2c (note the .re2c suffix) - This file contains a comment section which describes the JSON lexic. This section is translated by re2c into file src/json.hpp which is plain "vanilla" C++11 code. (In fact, the generated lexer consists of some hundred lines of gotos, which is a hint you never want to edit this file...).

If you only edit file src/json.hpp (without the .re2c suffix), your changes will be overwritten as soon as the lexer is touched again. To generate the src/json.hpp file which is actually used during compilation of the tests and all other code, please execute
```
make re2c
```
To run re2c and generate/overwrite file src/json.hpp with your changes in file src/json.hpp.re2c.
test/src/unit.cpp - This contains the Catch unit tests which currently cover 100 % of the library's code.

If you add or change a feature, please also add a unit test to this file. The unit tests can be compiled with
```
make
```
and can be executed with
```
./json_unit
```
The test cases are also executed with several different compilers on Travis once you open a pull request.

Please understand that I cannot accept pull requests changing only file src/json.hpp.

Note

If you open a pull request, the code will be automatically tested with Valgrind's Memcheck tool to detect memory leaks. Please be aware that the execution with Valgrind may in rare cases yield different behavior than running the code directly. This can result in failing unit tests which run successfully without Valgrind.

Please don't

Only make changes to file src/json.hpp -- please read the paragraph above and understand why src/json.hpp.re2c exists.
The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.8 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
Please do not open pull requests that address multiple issues.

coveralls · 2016-10-19T12:58:50Z

Coverage remained the same at 100.0% when pulling 06d60de on theodelrieu:feature/user_defined_types into e310850 on nlohmann:develop.

gnzlbg · 2016-10-19T18:11:21Z

src/json.hpp

+
+template <typename T>
+using remove_reference_t = typename std::remove_reference<T>::type;
+


you probably want uncvref_t here as well.

gnzlbg · 2016-10-19T18:15:00Z

src/json.hpp

+};
+
+template <typename T>
+constexpr T __static_const<T>::value;


I remember that identifiers starting with an underscore followed by adjacent underscores are reserved for the implementation in any scope (not only the global one), that is, one cannot use them. But the rules for reserved identifiers were more complex than that (this is C++ after all), so might want to check if that is actually allowed in a non-global scope (we are inside a namespace here).

According to cppreference : "the identifiers with a double underscore anywhere are reserved"

I'll rename it

gnzlbg · 2016-10-19T18:18:39Z

src/json.hpp

- : m_type(value_t::string), m_value(val)
- {
- assert_invariant();
+ basic_json(const string_t &val) : m_type(value_t::string), m_value(val) {


My intuition says that we would like to use the trait system for constructing basic_json objects from all types that are not basic_json, since in case somebody has a different kind of representation in a string (e.g. XML), they could still define their own trait class to construct basic_json objects from that.

I agree, although this would be a huge change to the current implementation, it might be out of the scope of this PR.

While I think we probably don't want to do this for string and int, and other primitive types yet, I recall that there was a constructor for "ranges" (types with value_type, begin and end). I think it would be still a good idea to at least move this constructor out to check that ADL works.

Are you talking about the constructor taking CompatibleArrayType& as a parameter?
Could you post a short pseudo-code about what you have in mind?

gnzlbg · 2016-10-19T18:19:33Z

src/json.hpp

@@ -1241,15 +1336,14 @@ class basic_json

 @sa @ref basic_json(const typename string_t::value_type*) -- create a
 string value from a character pointer
- @sa @ref basic_json(const CompatibleStringType&) -- create a string value
+ @sa @ref basic_json(const CompatibleStringType&) -- create a string
+ value


nitpick: you should probably revert all unnecessary formatting changes, since they make going through the relevant parts of the diffs harder

Indeed, forgot to check that, my bad

gnzlbg · 2016-10-19T18:41:05Z

src/json.hpp

+struct has_json_traits
+{
+ static constexpr bool value = has_destructor<json_traits<T>>::value;
+};


Do we need all this?

I think we can have some member function like this in basic_json:

// Sketch template <typename T> auto from_impl(T const& t, long) -> decltype(do_something_without_trait(t)); template <typename T> auto from_impl(T const& t, int) -> decltype(json_trait<T>::from_json(t, *this)); template <typename T> auto from(T const& t) -> decltype(from_impl(t, 0));

Notice how from calls from_impl(t, (int)0), which will try to call the overload from_impl(T, int) (which is a better match that the overload with long). If that works, then that is the one that is called. But if in that overload json_trait<T>::from fails, e.g., because there is no specialization for T, that overload will be silently removed, and then the second overload from_impl(T, long) will kick in, because there is an implicit conversion from int to long.

So what that does is "try to call the int overload, and if that fails, fall back to the long overload". You can probably use this "idiom" to easily detect whether json_traits implement something that you need, and fall back to some other solution if it doesn't.

(If you want to check some examples that actually work, a good example is how begin and end are implemented in range-v3, or in STL2).

If I understand correctly, the from_impl method aims to replace the two get_impl overloads that I added? I agree this is a lot more readable than what I have now.

gnzlbg · 2016-10-19T18:42:55Z

src/json.hpp

+ // Even if users can use the get<T> method to have a more 'functional' behaviour
+ // i.e. having a return type, could there be a way to have the same behaviour with from_json?
+ // e.g. auto t = nlohmann::from_json<T>(json{});
+ // this seems to require variable templates though... (at least it did when I tried to implement it)


I'll think about this, you make good points here.

gnzlbg · 2016-10-19T18:45:06Z

Thanks for working on this. I will try to take a more detailed look tomorrow at it, but the tests look good :)

theodelrieu · 2016-10-20T10:23:59Z

I'm also still asking myself whether I prefer the JSONDeserializer template argument, or the json_traits struct in the nlohmann namespace.

It would be nice to have a discussion about the pros and cons of both methods.

gnzlbg · 2016-10-20T10:29:02Z

I think @d-frey wanted to be able to serialize/deserialize the same type in different ways to json objects. I agree with him in that to do that the best thing to do is to have a template parameter that controls this in basic_json<..., JSONSerialization = json_traits>, but obviously we should give it a good default value that does the right thing for the 99% of the time in which one doesn't care.

We could also, inside basic_json, try to use the trait first, and if that fails, try to fall back to ADL. So that users can always use ADL but have an easy way to override it (e.g. they would only need to define a trait for the particular type they want to override, but everything else would still work through ADL in the same way).

It takes a lot of time and effort (and rewrites) to experiment with these things, but I guess once we get a bit of experience with it the best solution would become more clear.

theodelrieu · 2016-10-20T11:18:52Z

Good point, I shall experiment with the current way first then. Thanks!

theodelrieu · 2016-10-20T16:52:48Z

Hmpf, it seems VS2015 struggles with ADL... It compiles after specifying ::udt::from_json in the decltype. I hope there is a workaround

nlohmann · 2016-10-20T17:18:22Z

I'm really amazed about the effort and the discussion. I won't find the time to have a deeper look at this in the next days, but I am looking forward to! Keep up the great work!

coveralls · 2016-10-20T18:40:59Z

Coverage remained the same at 100.0% when pulling db4806b on theodelrieu:feature/user_defined_types into e310850 on nlohmann:develop.

coveralls · 2016-10-20T19:40:37Z

Coverage remained the same at 100.0% when pulling 9d290c6 on theodelrieu:feature/user_defined_types into e310850 on nlohmann:develop.

coveralls · 2016-10-20T20:40:51Z

Coverage remained the same at 100.0% when pulling 9d290c6 on theodelrieu:feature/user_defined_types into e310850 on nlohmann:develop.

coveralls · 2016-10-20T20:40:51Z

Coverage remained the same at 100.0% when pulling 9d290c6 on theodelrieu:feature/user_defined_types into e310850 on nlohmann:develop.

theodelrieu · 2016-10-22T17:18:40Z

Like all compiler workarounds, this one is a bit surprising...

coveralls · 2016-10-23T16:23:47Z

Coverage remained the same at 100.0% when pulling b81c2a2 on theodelrieu:feature/user_defined_types into e310850 on nlohmann:develop.

theodelrieu · 2016-10-26T10:13:07Z

I tried to provide some overloads for commonly used types, however it needs to rewrite a lot of code so I discarded that.
I think it's best to provide the functionality first, then enhance it later, if needed.

theodelrieu · 2016-10-26T10:23:45Z

Also, before trying to implement the JSONDeserializer way, I have a question for you @gnzlbg @nlohmann.
In order to give more control to users, I think the json_traits struct needs to be declared inside the basic_json class.

This allows to specialize on multiple basic_json instances, one where StringType = std::wstring for example.

One of the cons is readability though:

template <>
template <>
struct basic_json<>::json_traits<MyType>
{};

Is it worth going so far now? Should we support this after a first release?
I admit I don't really know where to go from here, (from the functionality-side), suggestions are welcome!

gnzlbg · 2016-10-26T12:55:20Z

I don't know if I understood you correctly, but if what you mean is that one user might want to provide different behavior in the from/to methods of json_traits depending on the type parameters of the basic_json object I still think that json_traits does not need to be a member of basic_json:

template <typename T> struct json_traits {};

template <>
struct json_traits<MyType> { 
   template <typename JSON>
   static void from(JSON const& j, MyType& m) {
     if constexpr (Same<string_type_t<JSON>, std::wstring>) {
       // do something if the string type is std::wstring
     } else {
       // do something else otherwise
     }
   }

   template <typename JSON>
   static void to(MyType const& m, JSON& j) { ... }
};

I just used a concept (Same<T, U...>) to branch on the string_type_t of basic_json (which I don't know if it is provided) using if constexpr, but one could use std::is_same_v, tag-dispatching, overloading, ... to do the same in C++14/11.

Commodity template aliases like json::string_type_t<basic_json<...>> to obtain the template parameters of a basic_json type is something we probably want to offer anyways. Not that they will be needed often, but for doing this kind of "very specialized things" you might need them.

theodelrieu · 2016-11-02T10:23:54Z

Thanks for your reply. Sorry I didn't have time to check the PR last week, I will try to work on it this week.
I don't think there is much left to do on this one, (implementation-wise).

I will start another PR for the other implementation.

bowling8 · 2016-11-02T15:15:18Z

I didn't see in the documentation...Is there functionality to convert libconfig to json? Any desire to add that functionality?

nlohmann · 2016-12-18T19:38:27Z

Continued in #355.

conversion from/to user-defined types

Théo Delrieu and others added 7 commits October 19, 2016 00:00

add first version support for user-defined types

c136f67

fix compilation on gcc-6

2ee7399

anonymous namespace renamed to detail

0001032

wip: add free function to_json/from_json support

63241db

wip: modify from_json prototype

f805dfc

wip: add get_impl overload that uses free from_json method

733e95f

add alias templates to reduce boilerplate

06d60de

theodelrieu mentioned this pull request Oct 19, 2016

Add a customization point for user-defined types #328

Closed

gnzlbg reviewed Oct 19, 2016

View reviewed changes

nlohmann added the kind: enhancement/improvement label Oct 20, 2016

Théo DELRIEU added 3 commits October 20, 2016 13:37

revert useless formatting

2407833

rename __static_const to _static_const (reserved identifier)

db4806b

use uncvref_t<T> instead of remove_cv_t<remove_reference_t<T>>>

9d290c6

theodelrieu force-pushed the feature/user_defined_types branch from 5544568 to 9d290c6 Compare October 20, 2016 12:06

remove has_destructor and has_json_traits, use decltype instead

5451aa5

Théo DELRIEU added 2 commits October 21, 2016 16:28

move most SFINAE trickery in to/from_json_fn

15507d1

fix compilation on vs2015 (taken from MS-ranges)

56492ce

fix compilation on gcc 4.9

b81c2a2

nlohmann added kind: improvement and removed kind: improvement labels Nov 13, 2016

orcun mentioned this pull request Dec 7, 2016

Serializing json instances containing already serialized string values without escaping #385

Closed

nlohmann closed this Dec 18, 2016

nlohmann added a commit that referenced this pull request Jan 27, 2017

🔀 merge pull request #435 (based on #338, #355, and #423)

42fa3f0

conversion from/to user-defined types

theodelrieu deleted the feature/user_defined_types branch March 13, 2019 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/user defined types #338

Feature/user defined types #338

theodelrieu commented Oct 19, 2016

coveralls commented Oct 19, 2016

gnzlbg Oct 19, 2016

gnzlbg Oct 19, 2016

theodelrieu Oct 20, 2016

gnzlbg Oct 19, 2016

theodelrieu Oct 20, 2016

gnzlbg Oct 20, 2016

theodelrieu Oct 20, 2016

gnzlbg Oct 19, 2016

theodelrieu Oct 20, 2016

gnzlbg Oct 19, 2016

theodelrieu Oct 20, 2016

gnzlbg Oct 19, 2016

gnzlbg commented Oct 19, 2016

theodelrieu commented Oct 20, 2016

gnzlbg commented Oct 20, 2016

theodelrieu commented Oct 20, 2016

theodelrieu commented Oct 20, 2016

nlohmann commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016

theodelrieu commented Oct 22, 2016

coveralls commented Oct 23, 2016

theodelrieu commented Oct 26, 2016

theodelrieu commented Oct 26, 2016

gnzlbg commented Oct 26, 2016 •

edited

Loading

theodelrieu commented Nov 2, 2016

bowling8 commented Nov 2, 2016

nlohmann commented Dec 18, 2016


		template <typename T>
		using remove_reference_t = typename std::remove_reference<T>::type;

Feature/user defined types #338

Feature/user defined types #338

Conversation

theodelrieu commented Oct 19, 2016

Files to change

Note

Please don't

coveralls commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnzlbg commented Oct 19, 2016

theodelrieu commented Oct 20, 2016

gnzlbg commented Oct 20, 2016

theodelrieu commented Oct 20, 2016

theodelrieu commented Oct 20, 2016

nlohmann commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016

coveralls commented Oct 20, 2016

theodelrieu commented Oct 22, 2016

coveralls commented Oct 23, 2016

theodelrieu commented Oct 26, 2016

theodelrieu commented Oct 26, 2016

gnzlbg commented Oct 26, 2016 • edited Loading

theodelrieu commented Nov 2, 2016

bowling8 commented Nov 2, 2016

nlohmann commented Dec 18, 2016

gnzlbg commented Oct 26, 2016 •

edited

Loading