remove stringstream dependency #1117
Conversation
| @@ -6,12 +6,10 @@ | |||
| #include <cmath> // ldexp | |||
| #include <cstddef> // size_t | |||
| #include <cstdint> // uint8_t, uint16_t, uint32_t, uint64_t | |||
| #include <cstdio> // snprinft | |||
| return ss.str(); | ||
| char cr[3]; | ||
| snprintf(cr, 3, "%.2X", current); | ||
| return std::string{cr}; |
There was a problem hiding this comment.
This might be better for NRVO purposes if you do it like this (untested, just speculation)
std::string cr{" "};
snprintf(&cr[0], 3, "%.2X", current);
return cr;
There was a problem hiding this comment.
Except that C++11 made that undefined, and the non-const data() doesn't exist until C++17, so never mind.
There was a problem hiding this comment.
Even with non-const .data() it is undefined.
The pointer is such that the range [data(); data() + size()) is valid
The main problem is the null-termination of snprintf, which always results in a string with an +1 increased stringlength compared to the the string created before.
There was a problem hiding this comment.
That's fine, size() is 2, so cr[0] to cr[2] is valid, and that's exactly what snprintf with a size of 3 will write to, including the nul terminator. In any case, we can't use it, as we can only require C++11.
should fix the GCC unittest
| ss << std::setw(2) << std::uppercase << std::setfill('0') << std::hex << static_cast<int>(byte); | ||
| JSON_THROW(type_error::create(316, "invalid UTF-8 byte at index " + std::to_string(i) + ": 0x" + ss.str())); | ||
| std::string sn(3, '\0'); | ||
| snprintf(&sn[0], sn.size(), "%.2X", byte); |
There was a problem hiding this comment.
How about changing this to:
snprintf(&sn[0], sn.size(), "%.2X", static_cast<uint8_t>(s[i]));
In order to avoid "‘%.2X’ directive output may be truncated writing between 2 and 8 bytes into a region of size 3 [-Wformat-truncation=]" (GCC 7.3.0).
There was a problem hiding this comment.
I don't see the difference.
byte is defined as
const auto byte = static_cast<uint8_t>(s[i]);
So it is 1 byte and the same, what you wrote.
Maybe a false positive in GCC?
There was a problem hiding this comment.
I suppose it might be a false positive since there is no difference.
Maybe someone with the same GCC version can confirm this warning.
Hi,
this PR replaces the
stringstreamconversions withsnprintfones and removes all stringstream dependencies.snprintfis already use in the project.Issue reference: #1111