Replace remaining sprintf with snprintf #9867

This is a little bit sketchy. Could it not safely be 2 here, not 3?

NUL term needs a 3rd byte.

For others http://www.cplusplus.com/reference/cstdio/snprintf/

A terminating null character is automatically appended after the content written.

Hence, 2 hex characters + 1 null character.

It's correct, and it's also why snprintf isn't an especially safe function call. (replaces one class of issue with another more subtle one)

It's correct, and it's also why snprintf isn't an especially safe function call. (replaces one class of issue with another more subtle one)

Indeed, that's why we never use it. The go-to function we have for this is "strprintf". However, it's kind of high overhead for formatting two hex characters. Especially as this function is called a lot.

I'll just replace this with use of HexDigit instead.

why not size_t?

Meh. For loop counters it's generally enough to use a 32-bit type. Especially if you know you're only going to count to 64, ever.

edit: fail

But incorrect.

nit: maybe emplace rather than push (potentially saves an initialization) @dcousens can you not delete your feedback? I had to dig in my emails to see that I wasn't repeating your feedback.

Do you really think there's anything to be gained by emplacing a char? I'd say something has to be really wrong for that to make a difference.

Anyhow I have a better solution based on reverse_iterator coming, so no need to discuss this further.

I'm pretty sure that the copy constructor and move constructor for a char are the same, so emplace wouldn't make a difference.

@JeremyRubin I made a woefully incorrect suggestion in regards to the bitmasking.

retval.push_back(hex_chars[value & 0xf0]);
retval.push_back(hex_chars[value & 0x0f]);

@dcousens no worries we all have our dumb moments.

Possibly we could even use HexStr here with a reverse_iterator, though I don't know how to do that for a C-style array. All the variations I can think of are much worse than just replicating this functionality here. Edit: found it out, it's simple!

#include "stdio.h"
#include <iterator>
int main() {
    char a[2] = {'a', 'b'};
    auto start = std::reverse_iterator<char *>(&a[2]);
    auto end = std::reverse_iterator<char *>(&a[0]);
    while (start != end)
        printf("%c", *(start++));
}

Outputs:

ba

Yes, thanks, I'm aware, see the new version of this patch.

ha -- and here I was thinking you coded it after I commented and I was like "wow, @laanwj is a god amongst men in code response times" ;)

Yes, I implemented it in minus 3 minutes! Truly godlike :-)