snippetcppMinor

Use va_list to format a string

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

formatting codereview cpp c++stackoverflow c++11 variadic strings

va_listusestringformat

Problem

I wrote a function that basically works like sprintf, but instead returns the formatted string (std::string). I haven't worked on this long, and I'm aware there are tons of improvements that could be made (alloc/realloc perhaps?). I was just looking for some advice before taking the next step.

GLOBAL string format(const string& format, ...)
{
    LOCAL const size_t initialSize = 64;

    string returnVal;

    char buffer[initialSize];
    int length;

    va_list args;

    va_start(args, format);
    {
        length = vsnprintf(buffer, initialSize, format.c_str(), args);
    }
    va_end(args);

    char bufferCorrectSize[length];

    va_start(args, format);
    {
        vsnprintf(bufferCorrectSize, length + 1, format.c_str(), args);
    }
    va_end(args);

    return bufferCorrectSize;
}

Ignore GLOBAL and LOCAL. They are just aliases for static I defined for organization.

Solution

Don't invent your own language through macros

GLOBAL and LOCAL might look cute but they make the code harder to read for everyone else. Not to mention the name-space pollution caused by such macros.

You say that they both expand to static. That seems wrong. A function declared static is not “very global” – it is private to its translation unit. A local variable declared static is not “very local” – it is shared between all invocations of the function. This also seems wrong. If you want to use format as a library function, it shouldn't be static. And declaring the integer initialSize as static seems pointless at best.

#include all required headers

Probably you have just left it out of your question but your code should always #include all required header files. In your case, that would be

` for std::vsnprintf,

for std::string,

for std::size_t and

for va_start, va_end and std::va_list.




Since you're using

string without the std:: qualifier, I assume you also have a using namespace std;

 somewhere in your code. Get rid of it and type the extra five characters instead. It makes it clearer where the types are coming from and doesn't pollute the global name-space.

Use

namespace

s to organize your functions

Maybe you just didn't show it but you should put everything except

main in some namespace. Instead of declaring stuff static, consider using an anonymous namespace. But since your function ought to be useful in multiple translation units, it probably shouldn't be static

 anyway.

Properly allocate and deallocate dynamic memory

The line

char bufferCorrectSize[length];


is not correct. (Your compiler should have warned you about it.) In C++, an array must have a fixed size known at compile-time. Since the value of

length

 is only obtained at run-time from a previous function call, this is not possible. You could dynamically allocate a buffer of the wanted size using

char * bufferCorrectSize = new char[length];


and release it again once you're done with it.

delete[] bufferCorrectSize;


But this leaves you with all kinds of memory management issues. So instead of managing it yourself, use a

std::vector

.

Unfortunately, you cannot use the

std::string directly because its data member function will only give you a const

 buffer. If you know exactly what you're doing, you can take the address of the first character of the string and use it a s a pointer to a writable buffer. But I'd rather not play such tricks unless you're comfortable with what you're doing and the additional performance is critical.

Don't repeat yourself

You're calling

std::vsnprintf

 twice; basically with the same arguments. Also, there is no need to call it again if the buffer was large enough the first time. You can re-factor this into a loop.

Get rid of the useless nested scope

While a nested scope

{ … }

 can be useful from time to time, in your case, it serves no purpose. You're not even declaring any variables inside it.

Handle errors properly

If

std::vsnprintf encounters an error, it will return a negative number. You should test for this and properly handle the error. throwing an exception is probably an adequate reaction. Don't forget to still call va_end

 in this case.

Handle embedded NUL bytes correctly

You're

returning bufferCorrectSize which will decay to char . Unlike others have commented, this is not returning a pointer to a local object. Since format is declared with a return type of std::string and the latter is implicitly constructible from a const char , you're safely returning the std::string

 as intended. However, if there is a NUL byte somewhere in the buffer, you'll truncate. Therefore, you should use the constructor taking a pointer and a length and call it explicitly.

Eliminate unused variables

The variable

returnVal

 is never used. Your compiler might be able to warn you about this.

Make debugging easier with function attributes

The C-style

printf

 family of functions has a bad reputation of being notoriously not type-safe and responsible for numerous bugs that can lead to serious security holes. A good compiler like GCC can warn you if such a function is called with arguments that do not match the type specifiers in the format string. You can enable the same warnings for your function, too. Of course, GCC can only check the arguments if it can see the format string. Using an untrusted format string is a very bad idea anyway so you should consider making it a

const char *

. If somebody really wants to call your function with a computed format string, they can always obtain a C-style string via the

c_str member function of std::string. This will give them the compiler warning they deserve so they can think twice about their decision. Having format accept a std::string` as format string also leads to the false assumption that one can embed NUL characters in it.

Code Snippets

char bufferCorrectSize[length];

char * bufferCorrectSize = new char[length];

delete[] bufferCorrectSize;

std::string
format(const char *const format, ...)
  __attribute__ ((format (printf, 1, 2)));

#ifndef MY_FORMAT_HXX
#define MY_FORMAT_HXX

#include <string>  // std::string

namespace my
{

  /**
   * @brief
   *         Like `std::sprintf` except that the formatted string is `return`ed
   *         as a `std::string`.
   *
   * This function internally forwards to `std::vsnprintf`.  It accepts the
   * exact same parameters and has the exact same gotchas as that function
   * except that allocating a buffer of sufficient size is done automatically.
   *
   * @param format
   *         C-style format string
   *
   * @param ...
   *         any format arguments required by the given format string
   *
   * @returns
   *         the formatted string
   *
   * @throws std::runtime_error
   *         if `std::vsnprintf` fails
   *
   * @throws std::bad_alloc
   *         if insufficient memory is available
   *
   */
  std::string
  format(const char *const format, ...)
    __attribute__ ((format (printf, 1, 2)));

}  // namespace my

#endif  // #ifndef MY_FORMAT_HXX

Context

StackExchange Code Review Q#115760, answer score: 9

Revisions (0)

No revisions yet.