Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/basic json conversion #986

Merged
merged 2 commits into from
Mar 6, 2018

Conversation

theodelrieu
Copy link
Contributor

@theodelrieu theodelrieu commented Feb 27, 2018

Attempt to fix #977.

Details can be found in the commit message body.

There is a FIXME left in the code about the value_t::discarded flag. I don't know in which cases this flag can be encountered.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • 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.

@trilogy-service-ro
Copy link

Can one of the admins verify this patch?

@coveralls
Copy link

coveralls commented Feb 27, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 8711ec6 on theodelrieu:fix/basic_json_conversion into 1f3d2a3 on nlohmann:develop.

@theodelrieu theodelrieu force-pushed the fix/basic_json_conversion branch from b5d4693 to d5216ec Compare February 27, 2018 15:03
@theodelrieu
Copy link
Contributor Author

In the end I've adopted the same behavior that can be found in the rest of the code (i.e. default: break;).

@theodelrieu
Copy link
Contributor Author

@nlohmann Apparently it does not change anything coverage-wise. I did not see a test converting from a discarded value, do you know what it's supposed to do? If so, I can add one :)

@nlohmann
Copy link
Owner

The discarded value is a means to let the parser callback mark values which should be discarded instead of being stored in the DOM. There is no real use case to generate these values, but I added as many tests as I needed for full coverage. That said, feel free to add a test if required :)

@theodelrieu
Copy link
Contributor Author

Hmm, so trying to explicitly convert a discarded value should throw?

@nlohmann
Copy link
Owner

No. It should just be copied as is.

@theodelrieu
Copy link
Contributor Author

Oh right, converting to something else than basic_json already throws. My bad.

Before this patch, `basic_json` types with different template arguments
were treated as `CompatibleArrayType`. Which sometimes leads to recursive
calls and stack overflows.

This patch adds a constructor and a `get` overload to deal with
different `basic_json` types.
@theodelrieu theodelrieu force-pushed the fix/basic_json_conversion branch from d5216ec to 8711ec6 Compare February 27, 2018 15:48
@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

What is the state of this PR? Is it ready to merge?

@theodelrieu
Copy link
Contributor Author

Yep, apparently it did fix #977.

@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

OK. I'll check whether it also closes #972 tonight.

@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

This PR fixes #977 and #972.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this Mar 6, 2018
@nlohmann nlohmann added this to the Release 3.1.2 milestone Mar 6, 2018
@nlohmann nlohmann merged commit 6203061 into nlohmann:develop Mar 6, 2018
@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

Thanks a lot @theodelrieu !

@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

I added a regression test (not yet committed), and got the following warning when compiled with GCC and maximal warnings:

In file included from src/unit-regression.cpp:32:
../single_include/nlohmann/json.hpp:10836:5: error: noexcept-expression evaluates to ‘false’ because of a call to ‘static void ns::foo_serializer<T, typename std::enable_if<(! std::is_same<ns::foo, T>::value)>::type>::to_json(BasicJsonType&, const T&) [with BasicJsonType = nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long long int, long long unsigned int, double, std::allocator, ns::foo_serializer>; T = int]’ [-Werror=noexcept]
     basic_json(CompatibleType && val) noexcept(noexcept(
     ^~~~~~~~~~
src/unit-regression.cpp:82:17: error: but ‘static void ns::foo_serializer<T, typename std::enable_if<(! std::is_same<ns::foo, T>::value)>::type>::to_json(BasicJsonType&, const T&) [with BasicJsonType = nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char>, bool, long long int, long long unsigned int, double, std::allocator, ns::foo_serializer>; T = int]’ does not throw; perhaps it should be declared ‘noexcept’ [-Werror=noexcept]
     static void to_json(BasicJsonType& j, const T& value)
                 ^~~~~~~
cc1plus: all warnings being treated as errors

This is a false positive, right?

@theodelrieu
Copy link
Contributor Author

Either that or your serializer is missing noexcept constraints.

Could you post the serializer code?

@gregmarr
Copy link
Contributor

gregmarr commented Mar 6, 2018

This is saying that we should mark ns::foo_serializer<>::to_json as noexcept because the compiler has determined that it can't throw. It's warning that we're not taking advantage of a potential optimization.

@nlohmann
Copy link
Owner

nlohmann commented Mar 6, 2018

@gregmarr was right - adding noexcept to that function silenced the warning, see 476b2e0#diff-fd33adc37899b833d4a2c69d0ef251c1R82

@theodelrieu theodelrieu deleted the fix/basic_json_conversion branch March 6, 2018 21:49
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.

Assigning between different json types
5 participants