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

Replace use of standard IO from error handling #3239

Closed
theShmoo opened this issue Jan 3, 2022 · 10 comments · Fixed by #3244
Closed

Replace use of standard IO from error handling #3239

theShmoo opened this issue Jan 3, 2022 · 10 comments · Fixed by #3244
Assignees
Labels
release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@theShmoo
Copy link
Contributor

theShmoo commented Jan 3, 2022

In
https://github.com/nlohmann/json/blame/develop/include/nlohmann/detail/output/serializer.hpp#L598-L599
and
https://github.com/nlohmann/json/blame/develop/include/nlohmann/detail/output/serializer.hpp#L504-L505
std::stringstream is used.

Use fmt instead. Instead of:

std::stringstream ss;
ss << std::uppercase << std::setfill('0') << std::setw(2) << std::hex << (byte | 0);
JSON_THROW(type_error::create(316, "invalid UTF-8 byte at index " + std::to_string(i) + ": 0x" + ss.str(), BasicJsonType()));

use:

JSON_THROW(type_error::create(316, fmt::format("invalid UTF-8 byte at index {}: {:#04x}", i, byte), BasicJsonType()));
@nlohmann
Copy link
Owner

nlohmann commented Jan 3, 2022

(Removed the bug label)

The code parts you mentioned are just to create an exception message. We used std::snprintf before, but had some warnings in some exotic compiler IIRC. I think adding a thirdparty library here would be overkill, or did I miss anything?

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Jan 3, 2022
@theShmoo
Copy link
Contributor Author

theShmoo commented Jan 3, 2022

Thanks for your fast response!

Aren't you already using fmt?

The problem is that we can't use IO streams in our code base (as we use your library in Intel SGX)

@theShmoo
Copy link
Contributor Author

theShmoo commented Jan 3, 2022

A while ago you helped me add the define JSON_NO_IO to your library.
maybe we could also use this define here?

Oh and I checked. You are not using fmt. Sorry. -> I will edit the title of the issue

@theShmoo theShmoo changed the title Use fmt instead of stringstream Replace use of standard IO from error handling Jan 3, 2022
@nlohmann
Copy link
Owner

nlohmann commented Jan 3, 2022

Ah, now I understand. That's indeed a limitation. Maybe you can check what fmt is doing under the hood - I guess some printf magic.

@theShmoo
Copy link
Contributor Author

theShmoo commented Jan 3, 2022

I implemented a short version of their conversion.
They use (a more complex version) of something similar that this:
https://godbolt.org/z/9n48qPKa6

@nlohmann
Copy link
Owner

nlohmann commented Jan 3, 2022

Awesome! I will have a look tonight. Is there a way I add the compiler you use in our CI? Then I would have caught these error myself.

@nlohmann nlohmann self-assigned this Jan 3, 2022
@theShmoo
Copy link
Contributor Author

theShmoo commented Jan 3, 2022

That is very nice of you!

As Intel provides their own SGX version of the whole standard library I would not recommend to add this to your ci, I think this would not pay off.

I always check in on your new releases and try to use it in our Codebase. If I encounter some problems I will report it.

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: please discuss please discuss the issue or vote for your favorite option labels Jan 3, 2022
@nlohmann nlohmann added this to the Release 3.10.6 milestone Jan 3, 2022
@nlohmann
Copy link
Owner

nlohmann commented Jan 3, 2022

I fixed the issue in #3244. Let me know if this works.

@theShmoo
Copy link
Contributor Author

theShmoo commented Jan 4, 2022

Wow! Thanks! I will check it out :)

@theShmoo
Copy link
Contributor Author

theShmoo commented Jan 4, 2022

It works!

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants