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

Json should not be constructible with 'json*' #448

Closed
jaredgrubb opened this issue Feb 12, 2017 · 7 comments
Closed

Json should not be constructible with 'json*' #448

jaredgrubb opened this issue Feb 12, 2017 · 7 comments

Comments

@jaredgrubb
Copy link
Contributor

I had something like this:

        json* p;
        json j(p);

This was a bug (I need to dereference the pointer), but json constructor accepts it as a boolean type because it's finding this overload:

template<typename BasicJsonType>
void to_json(BasicJsonType& j, typename BasicJsonType::boolean_t b) noexcept

The problem with 'bool' is that it captures a lot of things. Classes with 'operator bool()' are also being caught.

I can think of some simple changes (SFINAE something), but I'm not sure how well they work with arbitrary BasicJsonType's.

@jaredgrubb
Copy link
Contributor Author

Maybe the default to_/from_json should be implemented just for the common C++ types (bool, std::string, std::map, std::vector) and not in terms of BasicJson::whatever_t, and if you specialize basic_json then you have to also manually opt-in to the right to_/from_json overloads?

@theodelrieu
Copy link
Contributor

If I remember correctly, there was a constructor for each BasicJsonType:: whatever _t prior to 2.1.0.

So I think we should keep those overloads for now, to keep backward compatibility.

To avoid this implicit conversion, I guess a simple std::is_same<typename BasicJsonType::boolean_t, T> in the enable_if will do the trick. It would be great to fix similar issues with different parameter types (integers and so on)

Unfortunately I don't have a laptop at hand until Sunday to test this

@nlohmann
Copy link
Owner

@theodelrieu I agree in both points, and I think I won't have time to fix this this week either.

@jaredgrubb
Copy link
Contributor Author

I have done this and it seems to work:

-template<typename BasicJsonType>
-void to_json(BasicJsonType& j, typename BasicJsonType::boolean_t b) noexcept
+template<typename BasicJsonType, typename T,
+    enable_if_t<std::is_same<std::decay_t<T>, typename BasicJsonType::boolean_t>::value, int> = 0>
+void to_json(BasicJsonType& j, T b) noexcept

I'm not 100% that the decay is necessary.

@nlohmann nlohmann self-assigned this Feb 19, 2017
@nlohmann nlohmann added this to the Release 2.1.1 milestone Feb 19, 2017
nlohmann added a commit that referenced this issue Feb 19, 2017
@nlohmann
Copy link
Owner

Fixed using @jaredgrubb 's proposal.

@theodelrieu
Copy link
Contributor

I don't think the decay is useful, but that's a minor thing.

Thanks for the fix!

@nlohmann
Copy link
Owner

I shall remove the decay then.

@nlohmann nlohmann reopened this Feb 20, 2017
nlohmann added a commit that referenced this issue Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants