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

Disable implicit conversions from json to std::initializer_list<T> for any T #1118

Closed
tlemo opened this issue May 31, 2018 · 6 comments
Closed
Labels
state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@tlemo
Copy link

tlemo commented May 31, 2018

This is a cherry pick for from #1116: I suspect most ambiguities related to implicit conversions from json to standard containers (and not only) can be avoided by expanding the SFINAE around basic_json::operator ValueType() to explicitly exclude conversions to any std::initializer_list specialization.

It should be an quick, easy and safe fix (even if it doesn't fully address the full range of issues described in #1116). I'd be happy to prepare a patch if it helps.

@nlohmann
Copy link
Owner

Can you give an example for this?

@tlemo
Copy link
Author

tlemo commented May 31, 2018

The code snippet in #1116 demonstrates the issue. The assignment case can be mitigated with SFINAE in most cases (for the specific vector case we can't "fix" the constructor case since vector has the "explicit vector(size_t)" ...)

json j = { 1, 2, 3 };

// works as advertised
std::vector<float> v = j;

// error: use of overloaded operator '=' is ambiguous (with operand types 'vector<float>' and 'json' (aka 'basic_json<>'))
// note: candidate functions:
//  operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
//  operator=(const vector& __x);
//  operator=(initializer_list<value_type> __l)
v = j;

@theodelrieu
Copy link
Contributor

I'm quite against such a change, even if std::initializer_list with values cannot be constructed without braces.

The change would fix vector I agree, but it's a coincidence that there is no other operator=, otherwise it would not fix anything.

Especially if we decide to follow #958, this will make it harder for users to transition to get() everywhere in their codebase.

I have a huge bias because of #958 though :p

@tlemo
Copy link
Author

tlemo commented Jun 1, 2018

Correct, this would be a band aid, not a complete and universal solution. But it should be an effective band aid for the common cases (standard containers)

Is your concern that by mitigating some of the practical issues with implicit conversions we'd weaken the rationale for a more radical change (like the ones proposed in #958 and #1116) ?

@theodelrieu
Copy link
Contributor

Is your concern that by mitigating some of the practical issues with implicit conversions we'd weaken the rationale for a more radical change?

Exactly.

That's not my decision to make however, I think I have expressed my opinion enough already, so I'll follow along the final decision anyway.

@stale
Copy link

stale bot commented Jul 1, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jul 1, 2018
@stale stale bot closed this as completed Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

3 participants