CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Description
operator T()
considered harmful
There's two subjects of importance that I'd like to tackle.
This is the first one, I'll open another issue quickly (hopefully).
The problem
There's been several issues in the past related to the implicit conversion operator that the library provides.
The current Readme demonstrates the following use-case:
// conversion: json -> person
ns::person p2 = j;
This will call json::operator T()
with T = ns::person
, so no problems here.
You'd expect the following code to always work too:
ns::person p2;
p2 = j;
And it works! Sometimes.
If ns::person
has a user-defined operator=
, it will break with the following message:
t.cpp: In function ‘int main(int, const char**)’:
t.cpp:14:7: error: ambiguous overload for ‘operator=’ (operand types are ‘ns::person’ and ‘nlohmann::json {aka nlohmann::basic_json<>}’)
p2 = j;
^
t.cpp:6:6: note: candidate: ns::person& ns::person::operator=(int)
ns::person& operator=(int) { return *this; }
^~~~~~~~
t.cpp:3:8: note: candidate: constexpr ns::person& ns::person::operator=(const ns::person&)
struct ns::person
^
t.cpp:3:8: note: candidate: constexpr ns::person& ns::person::operator=(ns::person&&)
Hard to understand that error, and it's not something that can be fixed by the library.
It's triggered by the compiler before any template machinery on our side.
Now with such code:
void need_precise_measurement(Milliseconds);
// later on
json j = json::parse(config);
need_precise_measurement(j);
It can fail in two cases:
- Someone adds an overload for
need_precise_measurement
. Compiler error. - Someone changes the type of the argument to
Nanoseconds
. Runtime error at best.
Implicit conversions
Implicit conversions make the library pleasant to use. Especially when implicitely converting to JSON.
That's because we have a templated non-explicit constructor with lots of constraints to correctly handle user types.
There's also a single type to convert to: nlohmann::json
, so everything works.
However, when converting implicitely from json, it's the complete opposite. If the compiler cannot decide which type it should implicit convert the nlohmann::json
value to, you get a compiler error.
And it could happen in a codebase months after writing the code, (e.g. when adding an operator=
to a type).
In the end the only consistent way to convert from a json value is to call json.get<T>
.
Proposed change
I propose to simply and completely remove operator T()
.
It will obviously break code, so it can only be done in a major version.
Of course this change cannot be adopted right away, so I propose to add a deprecation warning inside operator T()
, as we did for basic_json::basic_json(std::istream&)
overloads.
Then it would be reasonable to remove it in a future major version.