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 and fmt::lib's format_arg() #964

Closed
AlexandreBossard opened this issue Feb 8, 2018 · 7 comments
Closed

Json and fmt::lib's format_arg() #964

AlexandreBossard opened this issue Feb 8, 2018 · 7 comments
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) state: help needed the issue needs help to proceed state: please discuss please discuss the issue or vote for your favorite option

Comments

@AlexandreBossard
Copy link

Bug Report

jsonbug.zip

I use the glorious fmt::lib for my project. Since i got bored of typing code
like fmt::print("my json {}", jsonvar.dump(2));, i set myself to use the fmt customization point for user-defined type.

Normally, fmtlib trigger static_assert when unrecognized type are passed as arguments, asking you to defined format_arg() or an ostream<< overload.

Not with nhlomann::json. I get some horrible error messages with gcc (7.2.0) with an ambiguous call to a constructor of some fmtlib internals.

By digging fmt code, i found a macro which disabled implicit int conversion. Shooting in the dark, copy paste the macro, and passing nhlomann::json to it. Fixed!

I am not sure if this is a the fmtlib or json fault (both?). But looking for related issues in both project, i found this one #958, which kind of help me.

@theodelrieu
Copy link
Contributor

After hacking the library a bit with Alex we found that removing the operator T fixed the issue (We also had to replace every instance of std::is_convertible with std::is_constructible inside conversions/from_json.hpp).

Luckily we can workaround the problem as Alex said with the internal fmt macro, while waiting for the next major json version (if we decide to remove operator T())

@nlohmann
Copy link
Owner

nlohmann commented Feb 9, 2018

I'm not convinced that removing operator T() would be a good idea, but let's discuss this in #958. However, adding a macro to our library to switch it off would be a first step.

@nlohmann
Copy link
Owner

nlohmann commented Feb 9, 2018

Can this be closed as the error can be fixed by defining a macro in fmtlib?

@AlexandreBossard
Copy link
Author

I can not call that a fix, since you have to used an undocumented API from fmtlib. The real bug is that json somehow short circuit the fmt's static_assert. replacing the cute and informative message about customization point by compiler errors barf. I was lucky to find the workaround with the fmtlib. But I suspect someone would come across the same issue with different lib and not be so "fortunate".

@nlohmann
Copy link
Owner

nlohmann commented Feb 9, 2018

If the only fix is #958, then this issue will not be fixed for a while (if ever). :-(

@nlohmann nlohmann added state: please discuss please discuss the issue or vote for your favorite option state: help needed the issue needs help to proceed labels Feb 9, 2018
@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label Feb 25, 2018
@nlohmann
Copy link
Owner

Closing this for now.

@tesch1
Copy link

tesch1 commented Dec 10, 2019

@AlexandreBossard what was this glorious macro that disabled the implicit int conversion?!? is it still in fmt? I dont see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) state: help needed the issue needs help to proceed state: please discuss please discuss the issue or vote for your favorite option
Projects
None yet
Development

No branches or pull requests

4 participants