Add Common::String::vformat

[fingolfin]

This pull request adds a Common::String::vformat static method. This relates to Common::String::format as vsprintf relates to sprintf. The main advantage over vsprintf is that it puts the result into a Common::String, and in particular also takes care of buffer size issues and stuff.
In addition, some (but by far not all) uses of vsprintf and vsnprintf have been converted to use Common::String::vformat, mainly to demonstrate how this is done. I did not want to convert more instances before knowing what others think about this patch.

The main caveat is the use of va_copy, which is not completely portable; in particular, MSVC does not seem to provide it. However, there are well-known workarounds for this. Still, this change might cause portability issues, and to overcome these fully and properly, we may have to add dedicated code to configure.

[fingolfin]

I should mention that there is an alternative way to implement Common::String::vformat which avoids the va_copy issue: Namely, take a vs(n)printf implementation (e.g. the one in backends/platform/symbian/src/vsnprintf.h) and directly adapt it to produce output in a Common::String.
This then would also ensure identical behavior on all platforms.
Drawback: This adds some code size, and on small systems (read: the Nintendo DS) this could be rather painful.

[lordhoto]

Does the NDS toolchain feature va_copy? If we really want to be free of portability issues we could auto detect (in configure or where ever) whether there is a va_copy present and if that is present use that and if not we could fall back to a custom implementation.

[zeldin]

Why is there a va_copy here in the first place? The code itself doesn't use the varargs, so it doesn't need a copy. Just remove both the va_copy and the va_end, and pass args directly to vsnprintf.

[fingolfin]

@lordhoto yes, the NDS toolchain uses GCC, and the GCC stdarg.h defines va_copy (except in strict ansi mode), and as all GCCs I am aware off, it always defines __va_copy (which my patch is also checking for).

@zeldin the va_copy is necessary because we use args twice. And by the standard, va_end invalidates the va_list it is used on.

[lordhoto]

I think even if we didn't use va_end directly after every vsnprintf call we need va_copy, since according to my manpage:

"The functions vprintf(), vfprintf(), vsprintf(), vsnprintf() are equivalent to the functions printf(), fprintf(), sprintf(),
snprintf(), respectively, except that they are called with a va_list instead of a variable number of arguments. These functions
do not call the va_end macro. Because they invoke the va_arg macro, the value of ap is undefined after the call. See stdarg(3)."

stdarg(3) clarifies this: "If ap is passed to a function that uses va_arg(ap,type) then the value of ap is undefined after the return of that function."

[zeldin]

Yeah, I didn't notice that it was (sometimes) used a second time. The normal way to write a vXXX function is that you use the va_list once, and the caller does both va_start() and va_end().

[fingolfin]

Yup, that's also exactly how one has to use Common::String::vformat. It has essentially the same semantics as vs(n)printf :)

Don't be fooled by the va_end occuring in there, these are necessary whenever we had to create a copy of the va_list, to kill the copy.

[lordhoto]

Any news on this?

[fingolfin]

I am still waiting for feedback. I.e: Do we want this method at all? If so, does the approach seem OK, or would an implementation based on rewriting a vs(n)printf implementation be preferable?

[sev-]

Do we have that many usage instances to justify potential portability problems?

[fingolfin]

There are 25 instances of vsnprintf and vsprintf in engines/ and common/ right.
Anyway, that's part of the question, I guess. But note that every port using GNU C, MSVC, Intel C++ or clang is supported. The only port where I don't currently know whether the patch works as-is is Symbian; but there, too, I know that it can be made to work with just a 2-3 lines of code, it just requires a look at the system headers of the platform.
This is much less severe than the portability issue caused by vs(n)printf itself, which some of our ports (e.g. Symbian...) have to re-implement as the port SDK does not offer them. Which is a huge and complicated function.

Note that the alternative proposal I made does not involve va_copy and would get rid of the need of having vs(n)printf available on the port. Though for those ports that have it, it would mean an increase in code size; since most ports have it (including NDS), that seems less appealing to me. But again, it's a matter of weighting.

Finally, we can of course do without this API, and keep using vs(n)printf with fixed buffer sizes and without checks for buffer overflows. Fine by me, too (less work for me, in fact :)

[wjp]

I think vformat is a useful addition, and any remaining va_copy issues should be easy enough to handle as you said. I'd vote for merging this.

[wjp]

The recent comments (on e.g., 2f8e9b9) about snprintf/vsnprintf in MSVC not adding terminating zeroes are also good arguments to migrate more to these string (v)format functions.

[fingolfin]

Let's not forget about the 771 uses of sprintf in engines/ alone ... which is actually worse than the snprintf maybe missing a zero terminator (that one is easy enough to fix with a simple wrapper around _vs(n)printf which sets the n'th char to 0.

[fingolfin]

(But of course, fixing s(n)printf does not require String::vformat, so while an important point, it's not really a "pro" for this pull request ;)

[sev-]

OK, I say go for it.

[fingolfin]

Committed