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

Implicit conversion issues #442

Closed
ukhegg opened this issue Feb 1, 2017 · 3 comments
Closed

Implicit conversion issues #442

ukhegg opened this issue Feb 1, 2017 · 3 comments
Assignees
Milestone

Comments

@ukhegg
Copy link

ukhegg commented Feb 1, 2017

Hi!
I'm using your library in my projects and i really like it.
But on the other hand it's basic_object<> implicit conversions from string is a source of bugs and long debugging sessions. I had two such sessions today.
Take a look. Some time ago a had a settings_provider class and a test mock

class settings_provider{
   virtual std::string get_settings(int id) const;
};  
class settings_provider_mock : public settings_provider{
   MOCK_CONST_METHOD1(get_settings, std::string(_));
};

Then,when using this mock,i setup it to return some json-formatted string

 EXPECT_CALL(*sp_mock ,get_settings(_)).WillOnce(Return("{ \"key\"=\"value\" }"));

Some time later i changed settings_provider and corresponding mock interface to

class settings_provider{
   virtual basic_json<> get_settings(int id) const;
};
class settings_provider_mock : public settings_provider{
   MOCK_CONST_METHOD<N>(get_settings, basic_json<>(_));
};

But the EXPECT_CALL continued to compile,but now get_settings returns json object with value "{ "key"="value" }".
So, my suggestion is to make conversion from string(and maybe other types) explicit, to avoid such errors in future.

@theodelrieu
Copy link
Contributor

Hi @ukhegg.

I don't think we can mark the constructor explicit (especially since there is only one constructor for almost every type now), this would break everyone's code.

I know that implicit conversions are a source of bugs, but in this case the _json suffix seems to be the best way to fix that kind of issues

@szikra
Copy link

szikra commented Feb 2, 2017

I also run into a similar issue when first tried doing something like this:
string smsg((char*)buff, len); json jmsg{smsg}; or json jmsg = smsg; or something.

I think what would help, is adding a note in the documentation here

// create object from string literal
json j = "{ \"happy\": true, \"pi\": 3.141 }"_json;

// or explicitly
auto j3 = json::parse("{ \"happy\": true, \"pi\": 3.141 }");

Like:

// Note the '_json' at the end of the string 
json j = "{ \"happy\": true, \"pi\": 3.141 }";  /// will not parse std::string, you need to use json::parse for that

@nlohmann
Copy link
Owner

nlohmann commented Feb 3, 2017

Implicit conversion is one of the major aspect that makes the library usable. Unfortunately, here the "wrong" string-to-JSON conversion has been chosen. I shall add a comment to the documentation to make this clearer.

@nlohmann nlohmann self-assigned this Feb 3, 2017
nlohmann added a commit that referenced this issue Feb 3, 2017
@nlohmann nlohmann added this to the Release 2.1.1 milestone Feb 3, 2017
nlohmann added a commit that referenced this issue Feb 3, 2017
@nlohmann nlohmann closed this as completed Feb 4, 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

4 participants