Using printf-style formatting to set the contents of a std::string

Question

I wanted to place text formatted with a printf-type formatting string in a C++ string. I came up with the following method, which seems to work, but I haven't found anyone else admitting to using a similar technique, so I thought I would ask for reasons why I should not do this.

#include <stdio.h>
#include <stdarg.h>
#include <string>

void stringprintf(std::string &str, char const *fmt, ...) {
    va_list    ap;

    va_start(ap, fmt);
    size_t n = vsnprintf(NULL, 0, fmt, ap);
    str.resize(n + 1, '\0');
    char *pStr = (char *)(str.c_str());
    (void)vsnprintf(pStr, str.capacity(), fmt, ap);
    va_end(ap);
}

void dumpstring(std::string &str) {
    printf("Object: %08X, buffer: %08X, size: %d, capacity: %d\ncontents: %s==\n", 
        &str, str.c_str(), str.size(), str.capacity(), str.c_str());
}

int main(int argc, char **argv) {
    std::string str;

    dumpstring(str);
    stringprintf(str, "A test format. A number %d, a string %s!\n", 12345, "abcdefg");
    printf("%s", str.c_str());
    dumpstring(str);
    return 0;
}

This question appears to be off-topic because it belongs to http://codereview.stackexchange.com/ — P0W, Nov 30 '13 at 14:31

score 3 · Answer 1 · answered Nov 30 '13 at 15:14

3

Just use operator[] (i.e. &str[0]) instead of c_str(). No undefined behavior then.

vsnprintf(&str[0], str.capacity(), fmt, ap);

You should probably chop off the null terminator when you're done, because std::string doesn't need it.

str.pop_back();

answered Nov 30 '13 at 15:14

Benjamin Lindley

101,917
9
204
274

Thank you. I had forgotten that the string class overrode the array indexing operator. However, in my test program at least, &(str[0]) returns the same value as str.c_str(), so the two forms appear to be equivalent. Since c_str() seems to return a pointer to the internal buffer I think that you are wrong about the terminating NUL, but perhaps under certain circumstances a new, NUL-terminated, buffer is created just for c_str(). – Chris Barry Nov 30 '13 at 18:11
Anyway, I wouldn't want to spoil a good argument by doing anything as unsporting as looking at the library source code, so I will just leave it in my cruise missile guidance package and investigate further if the customer complains. Since the array method does not involve hoodwinking the compiler with a cast I can always blame the library writer for leaving an unsafe loophole. – Chris Barry Nov 30 '13 at 18:18
@ChrisBarry: `std::string` keeps an extra null terminator *after* the actual string characters. When you use `vsnprintf`, it is writing a null terminator into the string characters. So by not removing that null terminator, you have two of them, and `str.size()` will be `strlen(str.c_str()) + 1`. – Benjamin Lindley Nov 30 '13 at 19:03
Yes, I see. You're quite right. So my code leaves to terminating NUL characters following the string. I shall repair this profligacy forthwith. – Chris Barry Nov 30 '13 at 19:21
Arrgh! s/to/two/ It's a dodgy keyboard. Honestly. – Chris Barry Nov 30 '13 at 19:26
Looking at this more carefully I see that you are right. There are two versions of operator[, one which returns a constant and one which does not. The comment in the header file (gcc 4.8.1) states that the non-constant operator unshares the string, so this seems to be functionally different to c_str() and should (as you said) be safe to use in this way. I would expect resize() to unshare the string as well , but its comment does not say so. Possibly it depends on whether or not the new size is different from the old size. – Chris Barry Nov 30 '13 at 20:21
Hadn't we already established that I was right? – Benjamin Lindley Nov 30 '13 at 20:23
Only about the NUL, although I realise now that I did not understand the issue properly then. If n is the returned by the first call to snprintf then the string should be resized to n + 1. snprintf should then be called with a size of n + 1, as it always places a terminator and the size includes that. Once the formatted text is in the string buffer resize must be called again, this time with a size of n, so str.size() does not include the NUL. I think now that this is what you were saying. What I had not accepted was that c_str() and operator[ behaved differently, with only one safe. – Chris Barry Nov 30 '13 at 22:27

score 2 · Answer 2 · answered Nov 30 '13 at 14:37

The catch is here;

char *pStr = (char *)(str.c_str());
(void)vsnprintf(pStr, str.capacity(), fmt, ap);

Reading the C++11 draft spec; (seems similar in other C++ revisions)

21.4.7.1 basic_string accessors [string.accessors]
const charT* c_str() const noexcept;
const charT* data() const noexcept;
...
3 Requires: The program shall not alter any of the values stored in the character array

As far as I can tell, allocating the memory and creating a new string/assigning it from the buffer is your closest option.

score 0 · Answer 3 · answered Nov 30 '13 at 14:39

0

The code you have is wrong, since it invokes undefined behavior. You are sprintf()-ing onto the pointer that string::c_str() returns. But string::c_str() returns a pointer to const char, so you can't do that. Why not use a non-const char buffer and construct a string from it?

int string_printf(std::string &str, const char *fmt, ...)
{
    va_list args;

    va_start(args, fmt);
    int n = std::vsnprintf(NULL, fmt, args);
    va_end(args);

    char *buf = new char[n + 1];

    va_start(args, fmt);
    std::vsnprintf(buf, fmt, args);
    va_end(args);

    str = std::string(buf, n);
    delete[] buf;

    return n;
}

answered Nov 30 '13 at 14:39

It is indeed wrong, but it does (seem to) work. As for "Why not ...", well that involves an extra allocation from the heap and whilst the effect on the application would only be apparent to someone able to discriminate time intervals smaller than a microsecond, nonetheless the knowledge of the inefficiency would trouble my aesthetic sensibilities and I would not sleep peacefully at my desk. – Chris Barry Nov 30 '13 at 18:37
@ChrisBarry It doesn't work. It invokes undefined behavior, so **by definition it does not work.** Also, you sleep better having written incorrect code with undefined behavior than having written correct code with an extra allocation? What? – Nov 30 '13 at 18:43
Furthermore, don't worry about "efficiency". Creating format strings is already as slow as hell. That extra allocation won't do any more harm. Unless you profiled your application and found that this extra allocation is the bottleneck (I highly doubt that), you have absolutely no right to even think about the speed of this operation, ***let alone*** substitute incorrect code for correct one. That's just **utterly, inherently wrong and insane.** – Nov 30 '13 at 18:45
You are quite right. Your argument is solidly founded on morality, while mine, based on mere aesthetics, crumbles away. Dissolved by a weak acid. However, you might consider my latest response to Benjamin Lindley for a method which achieves my original aim, yet may still be in accord with your reading of holy writ. – Chris Barry Nov 30 '13 at 21:16
Your definition of "not working" might possibly benefit from a small infusion of pragmatism. Such an attitude could have you overwhelmed by an erupting volcano, rather than escape on an aircraft without a current C. of A. which **by definition** cannot fly. – Chris Barry Nov 30 '13 at 21:21
@ChrisBarry huh... what? It's not "overwhelming pragmatism", or if it is, then something's wrong with software development these days. You shall never be relying on UB, whether or not it appears to be working. It is always the wrong approach to rely on UB. Especially if the fix is trivial (such as in this case). – Nov 30 '13 at 21:29

Using printf-style formatting to set the contents of a std::string

3 Answers3