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

theodelrieu · 2017-01-09T00:43:14Z

Hi!

This PR is the follow-up of #355

I managed to replace a lot of constructors with from/to_json overloads, which helped me to remove some SFINAE boilerplate, and to add a new feature: giving full control on type serialization to whom needs it.

I advise reviewers to review commit per commit, I tried my best to separate everything.

Things that need to be done

Put back and adapt the docs (I got a heavy hand on the 'delete' button)
static assert message improvement
improve README (talk about specializing types supported by the library, improve clarity)
TODO fill this row if something comes to mind

coveralls · 2017-01-09T01:15:26Z

Coverage decreased (-0.3%) to 99.724% when pulling 4282130 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

gregmarr · 2017-01-09T02:00:32Z

src/json.hpp

-    {
-        return get_impl(static_cast<ValueType*>(nullptr));
+        -> decltype(this->get_impl(static_cast<ValueType *>(nullptr))) {
+      return get_impl(static_cast<ValueType *>(nullptr));


The brace here doesn't match the style of the rest of the library.

gregmarr · 2017-01-09T02:00:51Z

src/json.hpp

+{
+  if (!j.is_boolean())
+    throw std::domain_error("type must be boolean, but is " + type_name(j));
+  b = *const_cast<Json&>(j).template get_ptr<typename Json::boolean_t*>();


Is there no way to do this without a const_cast?

I think it can, adding const to the get_ptr template arg should be good

gregmarr · 2017-01-09T02:04:24Z

src/json.hpp

+{
+  if (!j.is_string())
+    throw std::domain_error("type must be string, but is " + type_name(j));
+  s = *const_cast<Json&>(j).template get_ptr<typename Json::string_t*>();


As in boolean version, can this be done without const_cast?

gregmarr · 2017-01-09T02:06:35Z

src/json.hpp

+    friend bool operator==(json_pointer const &lhs,
+                           json_pointer const &rhs) noexcept
+    {
+      return lhs.reference_tokens == rhs.reference_tokens;


Looks like this library uses 4 space indents.

I added run make pretty to the list of things to be done

gregmarr · 2017-01-09T02:07:14Z

src/json.hpp

    /// the reference tokens
    std::vector<std::string> reference_tokens {};
-               };
+    };


Something still seems off about this brace indentation.

coveralls · 2017-01-09T09:57:01Z

Coverage decreased (-0.3%) to 99.724% when pulling 183100f on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-09T10:28:47Z

Coverage decreased (-0.2%) to 99.803% when pulling f603e76 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-09T11:03:18Z

Coverage decreased (-0.2%) to 99.803% when pulling f603e76 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-10T22:17:01Z

Coverage decreased (-0.2%) to 99.803% when pulling 51acff5 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-13T02:17:17Z

Coverage decreased (-0.2%) to 99.803% when pulling 2f85d37 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-13T03:25:12Z

Coverage decreased (-0.2%) to 99.803% when pulling 2f85d37 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-13T04:11:31Z

Coverage decreased (-0.2%) to 99.803% when pulling 2f85d37 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-13T04:52:36Z

Coverage decreased (-0.2%) to 99.803% when pulling 2f85d37 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-14T01:36:39Z

Coverage decreased (-0.04%) to 99.961% when pulling fc16270 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

theodelrieu · 2017-01-14T01:55:21Z

I think the unit-udt.cpp tests are quite good. And the whole library uses the to/from_json machinery now, that adds even more testing to the feature.

If someone finds an edge-case, or something I missed, please let me know

theodelrieu · 2017-01-14T02:02:16Z

@nlohmann Hi Niels, do you think we could meet on Slack soon? I'd like to have your input on some things
(mainly documentation, the refactoring and some questions that are hidden in code comments right now)

See you soon!

coveralls · 2017-01-14T02:48:18Z

Coverage remained the same at 100.0% when pulling 3affbfa on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-14T03:18:56Z

Coverage remained the same at 100.0% when pulling 3affbfa on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-14T03:48:27Z

Coverage remained the same at 100.0% when pulling 3affbfa on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

theodelrieu · 2017-01-15T21:23:47Z

I added a bit of doc, but frankly I'm terrible at this, so I'd gladly take some help on this part!

But once the doc and examples are done, I think we can merge it.

EDIT:
There is one question remaining though, it's about the get method.

Right now we have this

json j;
auto v = json.get<std::vector<int>&>();
// decltype(v) == std::vector<int>
auto u = json.get<const std::vector<int>>();
//decltype(u) == decltype(v)

I uncvref the template types that are passed to the get method, which is why we have this behavior.

I think it doesn't make any sense, so I tried to remove that and add some static_asserts.

However, it broke the existing tests, in unit-pointer_access.cpp which calls get<const ptr_type>(),
it seems that there's a special behavior when get() is called with types that match those in the basic_json template argument list (boolean_t, number_integer_t, etc...)

So, should we keep this behavior for those types, and static_assert for the rest? Or do we just keep using uncvref_t?

coveralls · 2017-01-15T21:48:26Z

Coverage remained the same at 100.0% when pulling 85a1f29 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-17T00:26:59Z

Coverage remained the same at 100.0% when pulling 1cb3b59 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-17T10:05:50Z

Coverage remained the same at 100.0% when pulling 5c47ce3 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

coveralls · 2017-01-17T10:42:03Z

Coverage remained the same at 100.0% when pulling 5c47ce3 on theodelrieu:feature/fromtojson into 748250e on nlohmann:feature/fromtojson.

theodelrieu · 2017-01-21T17:05:42Z

Unless I missed something, it is ready to be merged (minus documentation).
@nlohmann I kept all the primitive_iterator_t methods in the end, they are needed to avoid the implicit conversion madness

coveralls · 2017-01-21T17:48:05Z

Coverage remained the same at 100.0% when pulling 8d2216f on theodelrieu:feature/fromtojson into 145188f on nlohmann:feature/fromtojson.

This can however not be done easily for value_t, since external_constructor depends on it, as is operator< which was moved outside basic_json too. This is not really an issue, since all basic_json classes share the same enum

coveralls · 2017-01-21T18:57:49Z

Coverage remained the same at 100.0% when pulling 9c6ef74 on theodelrieu:feature/fromtojson into 145188f on nlohmann:feature/fromtojson.

coveralls · 2017-01-22T13:53:25Z

Coverage remained the same at 100.0% when pulling 9f8b270 on theodelrieu:feature/fromtojson into 145188f on nlohmann:feature/fromtojson.

coveralls · 2017-01-22T14:36:21Z

Coverage remained the same at 100.0% when pulling 9f103d1 on theodelrieu:feature/fromtojson into 145188f on nlohmann:feature/fromtojson.

theodelrieu · 2017-01-22T14:45:09Z

@nlohmann Code-wise, I'm done! Ready to merge :)

nlohmann · 2017-01-23T08:23:30Z

Very nice! I see whether I can do the merge tomorrow.

nlohmann · 2017-01-24T14:23:40Z

Changes moved to #435 which will be merged once the tests complete. Thanks everybody for the hard work over the last months!

conversion from/to user-defined types

gregmarr reviewed Jan 9, 2017

View reviewed changes

theodelrieu force-pushed the feature/fromtojson branch 4 times, most recently from bed76d7 to 2f85d37 Compare January 12, 2017 23:01

theodelrieu force-pushed the feature/fromtojson branch 3 times, most recently from 8cc2423 to 7a7bc07 Compare January 14, 2017 00:51

theodelrieu force-pushed the feature/fromtojson branch from b023103 to 3affbfa Compare January 14, 2017 01:48

theodelrieu force-pushed the feature/fromtojson branch from 1cb3b59 to 3843fa3 Compare January 17, 2017 09:32

Théo DELRIEU added 3 commits January 21, 2017 16:14

use switchs where appropriate

d566bb8

move conjunction/disjunction to the top of detail

a6b0282

rename template argument: BasicJson -> Json

889b269

theodelrieu force-pushed the feature/fromtojson branch from ec671b5 to 8d2216f Compare January 21, 2017 17:03

Théo DELRIEU added 8 commits January 21, 2017 19:12

disable reserve tests when exceptions are disabled

708eb96

rename template argument Json -> BasicJsonType

7f35901

remove inline keyword from anonymous namespace (not needed)

40ba5a8

remove useless forward declaration of primitive_iterator_t

f997758

put back type_name in basic_json

7d771c7

This can however not be done easily for value_t, since external_constructor depends on it, as is operator< which was moved outside basic_json too. This is not really an issue, since all basic_json classes share the same enum

put uncvref_t, enable_if_t, value_t and operator<(value_t) in detail

37fd20b

use typename U = uncvref_t where appropriate

ba0b35f

add static_asserts/remove the need for CopyConstructible in get

9c6ef74

theodelrieu force-pushed the feature/fromtojson branch from 8d2216f to 9c6ef74 Compare January 21, 2017 18:12

Théo DELRIEU added 2 commits January 22, 2017 14:07

fix some warnings

9f8b270

add implicit conversions test (operator T())

9f103d1

theodelrieu mentioned this pull request Jan 22, 2017

Conversion operators not considered #369

Closed

nlohmann mentioned this pull request Jan 23, 2017

Feature/user defined types bis #355

Closed

nlohmann self-assigned this Jan 24, 2017

nlohmann mentioned this pull request Jan 24, 2017

conversion from/to user-defined types #435

Merged

nlohmann closed this Jan 24, 2017

theodelrieu deleted the feature/fromtojson branch January 24, 2017 14:34

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

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

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

Conversation

theodelrieu commented Jan 9, 2017 • edited Loading

Things that need to be done

coveralls commented Jan 9, 2017

gregmarr Jan 9, 2017

Choose a reason for hiding this comment

gregmarr Jan 9, 2017

Choose a reason for hiding this comment

theodelrieu Jan 9, 2017

Choose a reason for hiding this comment

gregmarr Jan 9, 2017

Choose a reason for hiding this comment

gregmarr Jan 9, 2017

Choose a reason for hiding this comment

theodelrieu Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

gregmarr Jan 9, 2017

Choose a reason for hiding this comment

coveralls commented Jan 9, 2017

coveralls commented Jan 9, 2017

coveralls commented Jan 9, 2017

coveralls commented Jan 10, 2017

coveralls commented Jan 13, 2017

coveralls commented Jan 13, 2017

coveralls commented Jan 13, 2017

coveralls commented Jan 13, 2017

coveralls commented Jan 14, 2017

theodelrieu commented Jan 14, 2017

theodelrieu commented Jan 14, 2017

coveralls commented Jan 14, 2017

coveralls commented Jan 14, 2017

coveralls commented Jan 14, 2017

theodelrieu commented Jan 15, 2017 • edited Loading

coveralls commented Jan 15, 2017

coveralls commented Jan 17, 2017

coveralls commented Jan 17, 2017

coveralls commented Jan 17, 2017

theodelrieu commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 22, 2017

coveralls commented Jan 22, 2017

theodelrieu commented Jan 22, 2017

nlohmann commented Jan 23, 2017

nlohmann commented Jan 24, 2017

theodelrieu commented Jan 9, 2017 •

edited

Loading

theodelrieu Jan 9, 2017 •

edited

Loading

theodelrieu commented Jan 15, 2017 •

edited

Loading