Feature/user defined types bis #355

theodelrieu · 2016-11-07T22:27:05Z

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

theodelrieu · 2016-11-07T22:41:00Z

This is a first version of the JSONSerializer template argument, (alternate implementation).
I didn't make a custom serializer in the tests yet. But I already feel that I like the first implementation more.

I find the need to have another basic_json typedef for types we want to serialize differently quite problematic.

A use case in my own code is partial specialization on template types (e.g. boost::optional, boost::containers, ...).

In order to rely on ADL, one would need to write a from_json method in the boost namespace, which is undesirable.

@gnzlbg I know you mentioned something about providing some "c++11 concepts" (e.g. Optional Concept), however I think we should provide UDT support first, then enhance it if users demand it.

What do you think about this ?

coveralls · 2016-11-08T00:04:12Z

Coverage remained the same at 100.0% when pulling 280cd36 on theodelrieu:feature/user_defined_types_bis into 9ca00e4 on nlohmann:develop.

gnzlbg · 2016-11-08T11:24:41Z

In order to rely on ADL, one would need to write a from_json method in the boost namespace, which is undesirable.

That would be undesirable but doing so is not necessary. If the customization point is implemented as suggested in the paper, then you have something like this (from the top of my head using my brain's imperfect C++ compiler):

namespace json {

// here you can define from_json for any type 

template <typename JSON, typename T, std::size_t N>
void from_json(JSON const& j, T (t&)[N]) {
  // implementation for C-Arrays
}

template <typename JSON, typename T>
void from_json(JSON const& j, boost::optional<T>& o) {
  // implementation for boost::optional
}

// if you have a type trait to select "Optional-like objects" you could have a 
// generic implementation for those here as well, but you just want it to
// be picked if a concrete implementation is not found

struct from_json_fn {
  template <typename JSON, typename T>
  constexpr auto operator()(JSON const& j, T& t) const noexcept(...) 
  -> decltype(from_json(j, t)) {
    return from_json(j, t);
  }
};

namespace { 
  static constexpr auto const& from_json = static_const<from_json_fn>::value;
}  // namespace 

}  // namespace json

The only thing to take into account is that you don't want nlohmann::json to depend on boost::optional, so this means you should put those overloads in the json namespace in a separate header, e.g., <json/extensions/boost/optional.hpp>, that people can then opt into including. IIRC the only condition is that those extension headers must be included before including json.

EDIT: to make it more clear, when from_json is called unqualified within from_json_fn::operator(), it will try to find a suitable overload both in scope (the global and the json namespaces), and the associated namespaces of the types involved, i.e., the associated namespace of JSON (which in this case should be json), and the associated namespace of T, which in case of boost::optional will be the boost namespace.

theodelrieu · 2016-11-08T12:03:53Z

It makes sense to have extensions separately, even if I believe we should add them later, if users show the need.

By the way, I re-checked one of your comments in issue #328 and I realized that I forgot to add template arguments to the JSONSerializer class. Which enables the use of partial specialization and ADL if both are wanted.

This solves the main issue I had with that solution!

coveralls · 2016-11-08T12:47:53Z

Coverage remained the same at 100.0% when pulling 4fae04c on theodelrieu:feature/user_defined_types_bis into 9ca00e4 on nlohmann:develop.

jaredgrubb · 2016-11-09T00:24:56Z

src/json.hpp

+  static auto from_json(Json&& j, T& val) -> decltype(::nlohmann::from_json(std::forward<Json>(j), val))
+  {
+    ::nlohmann::from_json(std::forward<Json>(j), val);
+  }


This is missing a 'return' and maybe you should have a unit test on a from_json type that does return a value.

IIUC from_json should never return a value, that is, its signature should be (JSON const& input, T& output) -> void, so maybe it would be better to omit the return value but put a void in the decltype like this:

static auto from_json(Json&& j, T& val) -> decltype(::nlohmann::from_json(std::forward<Json>(j), val), void) { ::nlohmann::from_json(std::forward<Json>(j), val); }

I came to the same conclusion, right now, the code is quite messy, I thought at first it would be a nice idea to return the type directly.

However, we cannot know which basic_json type will be used by the user in its own method, hence the need to pass it as an argument.

I will remove the overloads in a next commit

coveralls · 2016-11-10T00:24:47Z

Coverage remained the same at 100.0% when pulling 2530767 on theodelrieu:feature/user_defined_types_bis into 9ca00e4 on nlohmann:develop.

coveralls · 2016-11-10T11:00:33Z

Coverage remained the same at 100.0% when pulling 9ca5d9c on theodelrieu:feature/user_defined_types_bis into 9ca00e4 on nlohmann:develop.

theodelrieu · 2016-11-10T12:12:14Z

@nlohmann there seems to be a problem in a test.

I had an out of memory error once, in unit-constructor1.cpp, when constructing a JSON from a std::ifstream. Simply launching the executable from elsewhere solved it for me.

I don't know if the Travis' segfault is related though, do you have this problem on other PR?
It didn't trigger on each of my commit, that's why I'm wondering

nlohmann · 2016-11-11T16:34:41Z

Travis seems to have problems every now and then - I restarted the build and it succeeded.

theodelrieu · 2016-11-15T13:44:11Z

In the last commit I faced an issue when providing a user-defined JSONSerializer.

Since I did not change the way base types are handled, such as int or std::string, the following code breaks because of ambiguity, when doing enable_if checks:

// when not specialized, I want to use ADL as well
template <typename T, typename = void>
struct my_serializer
{
  template <typename Json>
  static void from_json(Json const& j, T& val)
  {
    nlohmann::from_json(j, val);
  }
};

There are some ways to solve this:

specialize the serializer for every type
do some SFINAE to avoid already handled types
handle basic types in nlohmann with to_json / from_json functions

The first two are quite annoying for users, and the latter requires a bit of refactoring (which will be needed someday if this feature is merged IMHO).

I would rather choose the latter, which doesn't impact users
@gnzlbg @nlohmann Do you see another way to solve this issue?

coveralls · 2016-11-15T14:36:57Z

Coverage remained the same at 100.0% when pulling 738157a on theodelrieu:feature/user_defined_types_bis into 9ca00e4 on nlohmann:develop.

gnzlbg · 2016-11-15T18:03:51Z

Do you see another way to solve this issue?

Not really. From the point of view of the new serialization/deserialization mechanism, primitive types and standard library types look like user-defined types :/

nlohmann · 2016-11-15T18:25:50Z

and the latter requires a bit of refactoring (which will be needed someday if this feature is merged IMHO).

About what kind of refactoring are we talking?

gnzlbg · 2016-11-15T18:29:33Z

The constructors / assignments of basic_json for int, float, std::string, ranges(begin, end, value_type) ... would be extracted to from_json/to_json free functions. The only "special" constructor / assignment that would remain would be a template that is constrained on the type T being JSONSerializable.

theodelrieu · 2016-11-16T09:13:22Z

I am working on a fourth solution, that does not require refactoring. It should be ready around tonight.

theodelrieu · 2016-11-16T22:26:18Z

After a lot of rewrites, I came to this implementation.

I managed to remove constraints on nlohmann::adl_serializer and the custom one in the tests, without breaking anything.

I wanted to have your feedback on it, I'm sorry I didn't write comments, so do not hesitate to comment on the diff!

This fourth solution has the advantages of the former discussed one, without its main drawback: the need to rewrite almost of the lib...

gnzlbg · 2016-11-16T22:44:58Z

src/json.hpp

+// taken from ranges-v3
+// TODO add doc
+template <typename T>
+struct _static_const


nitpick: the leading underscore might be undefined behavior (I always forget the rules...). Worth checking.

According to http://en.cppreference.com/w/c/language/identifier#Reserved_identifiers, it seems that you're right, since this qualify as an external identifier (never seen that term before).
Anyway, removing the underscore seems a good idea

I rechecked and IIUC this usage is fine because the underscore:

is not followed by another underscore,

is not followed by a capital letter,

and the identifier is not in the global namespace.

I am still not sure that I understood the rules correctly though... Avoiding identifiers that start with an underscore was always my pragmatic solution (Life is too short, etc.).

I'm still confused by the "external identifier" thing, but it would surprise me if identifiers beginning with an underscore (except the global namespace) were to be reserved.

I would be an outlaw with all my prefixed private variables...

gnzlbg · 2016-11-16T22:48:31Z

src/json.hpp

+                                                                                        detail::is_compatible_unsigned_integer_type<number_unsigned_t, uncvref_t<T>>::value or
+                                                                                        std::is_constructible<string_t, uncvref_t<T>>::value or
+                                                                                        std::is_base_of<std::istream, uncvref_t<T>>::value or
+                                                                                        std::is_same<boolean_t, uncvref_t<T>>::value), int> = 0>


Might want to abstract all these checks into an detail::is_basic_json_compatible<uncvref_t<T>> or something like that. It might make the function definition clearer, and it also might make it easier to explain what is going on.

Yes indeed, I pushed as soon as I made it work, I also forgot to format the relevant parts, sorry for that

gnzlbg

The overall approach looks good to me.

The whole thing still needs to be cleaned up, documented, I haven't reviewed the tests, etc., but a solution in this spirit should work for all situations discussed in the issue.

gnzlbg · 2016-11-16T22:50:01Z

src/json.hpp

-                 std::numeric_limits<CompatibleNumberUnsignedType>::is_integer and
-                 not std::numeric_limits<CompatibleNumberUnsignedType>::is_signed,
-                 CompatibleNumberUnsignedType>::type = 0>
+    template<typename CompatibleNumberUnsignedType, enable_if_t<detail::is_compatible_unsigned_integer_type<number_unsigned_t, CompatibleNumberUnsignedType>::value, int> = 0>


why do you need to pass number_unsigned_t to detail::is_compatible_unsigned_integer_type ? Shouldn't passing it CompatibleNumberUnsignedType be enough? (as in, can't we simplify this a bit?)

I think we can, I simply translated the existing checks, to avoid spending time on it (before making the whole thing work)

gnzlbg · 2016-11-16T22:50:13Z

src/json.hpp

-    template<typename CompatibleNumberFloatType, typename = typename std::enable_if<
-                 std::is_constructible<number_float_t, CompatibleNumberFloatType>::value and
-                 std::is_floating_point<CompatibleNumberFloatType>::value>::type>
+    template<typename CompatibleNumberFloatType, enable_if_t<detail::is_compatible_float_type<number_float_t, CompatibleNumberFloatType>::value, int> = 0>


same as a bove

gnzlbg · 2016-11-16T22:52:08Z

src/json.hpp

+                                                                                        detail::is_compatible_array_type<basic_json_t, uncvref_t<T>>::value or
+                                                                                        detail::is_compatible_float_type<number_float_t, uncvref_t<T>>::value or
+                                                                                        detail::is_compatible_integer_type<number_integer_t, uncvref_t<T>>::value or
+                                                                                        detail::is_compatible_unsigned_integer_type<number_unsigned_t, uncvref_t<T>>::value or


Same here, define a detail::is_compatible_basic_json_type (or similar) that abstracts all this and makes it more readable.

gnzlbg · 2016-11-16T22:52:57Z

src/json.hpp

+};
+
+void to_json();
+void from_json();


Why do we need these two here? are they really necessary?

I need to add a comment on that, this is the MSVC workaround taken from Microsoft range implementation

While reading all the has_xxxx classes I was wondering why you did just not use expression SFINAE for these, and actually wondered if it might had something to do with MSVC.

EDIT: Not that I think expression SFINAE is better, but it would be the laziest solution and what I would have tried at first.

It was more because of clarity at first. I try to avoid expression SFINAE when it becomes too demanding on MSVC too

gnzlbg · 2016-11-16T22:53:39Z

src/json.hpp

@@ -122,6 +135,186 @@ struct has_mapped_type
        std::is_integral<decltype(detect(std::declval<T>()))>::value;
 };

+template <template <typename...> class T>
+struct wrapper{};


Is this used somewhere?

nope I forgot that

coveralls · 2016-11-17T08:20:46Z

Coverage remained the same at 100.0% when pulling c4be803 on theodelrieu:feature/user_defined_types_bis into 9ca00e4 on nlohmann:develop.

theodelrieu · 2016-11-17T17:35:18Z

uuh? Is there a problem with travis? I dont see conflicts

EDIT: nevermind, I see them now

nlohmann · 2016-12-13T21:25:01Z

test/src/unit-class_const_iterator.cpp

@@ -220,20 +220,20 @@ TEST_CASE("const_iterator class")
            {
                json j(json::value_t::null);
                json::const_iterator it = j.cbegin();
-                CHECK(it.m_it.primitive_iterator == 1);
+                CHECK(it.m_it.primitive_iterator.m_it == 1);


Just out of curiosity: why is the .m_it suffix necessary?

Well this a consequence of removing the explicit keyword on the new constructor.

As I mentioned a few days ago:

In the second [example], the converting struct has template arguments, which prevents the sneaky implicit conversion, since the compiler cannot deduce T when trying to convert int to basic_json.

Although code outside the basic_json class is protected from this sneaky implicit conversion, the code inside is not!

Every implicit conversion inside basic_json will try to use the T && constructor, if it goes trough the enable_ifs.

It happened with primitive_iterator because there was an implicit conversion operator to difference_type, that's why I reimplemented the iterator operators (copy-pasted until I refactor, sorry about that).

That's also the reason why I made the .m_it suffix public, it was also needed in the tests, because Catch does fancy capture stuff, and seems to be able to call the doomed constructor (through a call to operator<<(ostream&, primitive_iterator) if I remember correctly)

I should have added comments explaining this, sorry about that too!

Thanks for the explanation. Sorry for not having seen this in the discussion above. Great work!

coveralls · 2016-12-14T01:23:30Z

Coverage remained the same at 100.0% when pulling 13082c1 on theodelrieu:feature/user_defined_types_bis into bc28942 on nlohmann:develop.

coveralls · 2016-12-14T03:45:08Z

Coverage remained the same at 100.0% when pulling 13082c1 on theodelrieu:feature/user_defined_types_bis into bc28942 on nlohmann:develop.

coveralls · 2016-12-14T22:33:20Z

Coverage remained the same at 100.0% when pulling b6ef4f7 on theodelrieu:feature/user_defined_types_bis into bc28942 on nlohmann:develop.

coveralls · 2016-12-15T11:56:01Z

Coverage remained the same at 100.0% when pulling 59ffbab on theodelrieu:feature/user_defined_types_bis into bc28942 on nlohmann:develop.

theodelrieu · 2016-12-15T21:37:24Z

Hi everyone, we're almost done!

@nlohmann, @gnzlbg and I will have a Slack meeting shortly, to discuss the design decisions, and decide where we go from here.

If you want to join, feel free to send me an email, I'll invite you to the Slack team!

I'd like to plan it for this weekend, if it fits everyone's agenda. Let me know and I'll post the meeting date on Slack and Github.

Thanks

nlohmann · 2016-12-15T21:46:58Z

Sunday or Friday evening (CET) would fit for me.

gnzlbg · 2016-12-16T09:51:39Z

Sunday fits me, today it depends on the time.

…

On Thu, Dec 15, 2016 at 10:47 PM, Niels Lohmann ***@***.***> wrote: Sunday or Friday evening (CET) would fit for me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#355 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NptoC7SJ1M8xoJHmsE8eRZbziN7y1ks5rIbVXgaJpZM4KrzJH> .

theodelrieu · 2016-12-16T10:10:37Z

Sunday is fine for me, I can't make it today unfortunately

nlohmann · 2016-12-18T08:56:30Z

How about 19:00 (Berlin time) today?

theodelrieu · 2016-12-18T09:45:23Z

No problem for me

theodelrieu · 2016-12-19T21:11:58Z

Hi, I was thinking about what we talked about yesterday, i.e. replacing existing basic_json constructors by to_json/from_json overloads.

This is not only about cleaning code, but this enables a feature:

In the code, we call JSONSerializer<T>::to_json in the new constructor, but there is a lot of enable_ifs to avoid chosing the others (when T = int for example).

The issue with those constraints is that users cannot override the default behaviour for vector, array or unscoped enums (and all types already handled by the library)

But if we change constructors to free functions, those functions would be called via the default adl_serializer<T>::from_json, thanks to ADL.

Which means, if a user specialized adl_serializer<std::vector<T>>, that his specialization will be called.
This allows to support unscoped enums, and to give full control to the users that need it.

Also, they would still be able to call the default behaviour:

std::vector<int> v{1, 2, 3};

// this is also called by the default adl_serializer too
auto j = nlohmann::to_json(v);

This might have been mentioned by @gnzlbg yesterday, I don't remember, but I thought it would be useful to share that thought.

gnzlbg · 2016-12-19T21:28:14Z

No, I didn't mention it (and hadn't really thought about it), but yes, you are correct, if everything goes through the trait system then the trait system would allow this kind of "override" (note: this kind of override is not possible with just ADL because adding overloads to std:: is undefined behavior).

theodelrieu · 2016-12-19T22:15:48Z

Indeed, but if we add the overloads in the detail namespace, it should work:

namespace nlohmann::detail {

// this should be found via unqualified call
template <typename JSON, typename T, typename Allocator>
void to_json(JSON&, std::vector<T, Allocator> const&) {}

struct to_json_fn {
template <typename JSON, typename T>
void operator()(JSON& j, T&& val) {
  // for T = std::vector<U>, this should call the overload ahead
  to_json(j, std::forward<T>(val));
}
};
} // nlohmann::detail

I haven't tried to compile this, but I think it's valid

gnzlbg · 2016-12-20T13:03:15Z

Yes that would be the way to move the "constructors" out of basic_json. What I meant was only that the only way for users to override this would be to either specialize the adl_serializer trait or pass basic_json a different serializer trait since they cannot add overloads to the namespaces associated with those types. I guess this should be documented.

nlohmann · 2017-01-23T08:24:17Z

This can be closed, right? After all, we have #423 now.

qis · 2017-01-23T08:32:43Z

Looks like it. Thank you very much for caring about this topic and not rushing things.

theodelrieu · 2017-01-23T08:54:00Z

Yes, this can be closed

conversion from/to user-defined types

jaredgrubb reviewed Nov 9, 2016

View reviewed changes

gnzlbg reviewed Nov 16, 2016

View reviewed changes

theodelrieu force-pushed the feature/user_defined_types_bis branch from 2dcffbb to e7bdc2f Compare November 17, 2016 17:16

theodelrieu force-pushed the feature/user_defined_types_bis branch from 7c60907 to d9a7493 Compare November 18, 2016 10:05

theodelrieu force-pushed the feature/user_defined_types_bis branch from 0e8e9b6 to 75ae715 Compare November 26, 2016 00:19

Théo Delrieu and others added 5 commits November 26, 2016 01:45

add first version support for user-defined types

ca811a2

fix compilation on gcc-6

8da5ad5

anonymous namespace renamed to detail

528315f

wip: add free function to_json/from_json support

d6fea62

wip: modify from_json prototype

2bb49e1

add more tests to unit-udt.cpp

f9e02e9

nlohmann reviewed Dec 13, 2016

View reviewed changes

add support for enum classes

13082c1

theodelrieu added 2 commits December 14, 2016 21:53

fix msvc, by doubling parenthesis on catch assertions

b6ef4f7

use u8 prefix in unit-udt.cpp

301af87

theodelrieu and others added 2 commits December 15, 2016 00:06

add more tests

35682b4

fix tests, avoid instantiating JSONSerializer when it will not be used

59ffbab

nlohmann mentioned this pull request Dec 18, 2016

Feature/user defined types #338

Closed

theodelrieu mentioned this pull request Jan 9, 2017

user-defined types ter: clean, refactor basic_json class #423

Closed

nlohmann closed this Jan 23, 2017

theodelrieu deleted the feature/user_defined_types_bis branch January 23, 2017 10:27

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

dota17 mentioned this pull request Feb 5, 2020

install Slack app #1904

Closed

Feature/user defined types bis #355

Feature/user defined types bis #355

Conversation

theodelrieu commented Nov 7, 2016

theodelrieu commented Nov 7, 2016

coveralls commented Nov 8, 2016

gnzlbg commented Nov 8, 2016 • edited Loading

theodelrieu commented Nov 8, 2016

coveralls commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

theodelrieu commented Nov 10, 2016

nlohmann commented Nov 11, 2016

theodelrieu commented Nov 15, 2016

coveralls commented Nov 15, 2016

gnzlbg commented Nov 15, 2016

nlohmann commented Nov 15, 2016

gnzlbg commented Nov 15, 2016 • edited Loading

theodelrieu commented Nov 16, 2016

theodelrieu commented Nov 16, 2016

Choose a reason for hiding this comment

theodelrieu Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

gnzlbg Nov 16, 2016 • edited Loading

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 left a 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 Nov 16, 2016 • edited Loading

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

coveralls commented Nov 17, 2016

theodelrieu commented Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 14, 2016

coveralls commented Dec 14, 2016

coveralls commented Dec 14, 2016

coveralls commented Dec 15, 2016 • edited Loading

theodelrieu commented Dec 15, 2016

nlohmann commented Dec 15, 2016

gnzlbg commented Dec 16, 2016 via email • edited Loading

theodelrieu commented Dec 16, 2016

nlohmann commented Dec 18, 2016

theodelrieu commented Dec 18, 2016

theodelrieu commented Dec 19, 2016

gnzlbg commented Dec 19, 2016 • edited Loading

theodelrieu commented Dec 19, 2016

gnzlbg commented Dec 20, 2016

nlohmann commented Jan 23, 2017

qis commented Jan 23, 2017

theodelrieu commented Jan 23, 2017

gnzlbg commented Nov 8, 2016 •

edited

Loading

gnzlbg commented Nov 15, 2016 •

edited

Loading

theodelrieu Nov 16, 2016 •

edited

Loading

gnzlbg Nov 16, 2016 •

edited

Loading

gnzlbg Nov 16, 2016 •

edited

Loading

theodelrieu commented Nov 17, 2016 •

edited

Loading

coveralls commented Dec 15, 2016 •

edited

Loading

gnzlbg commented Dec 16, 2016 via email •

edited

Loading

gnzlbg commented Dec 19, 2016 •

edited

Loading