Replace sprintf calls with stringstream

[datahead8888]

As discovered in #7 there are some calls to sprintf in the code.

Example - hud.cpp:

sprintf( text, _("Points %08d"), static_cast<int>(pLevel_Player->m_points) );

In order to be consistent with our decision to use cout/cerr rather than printf, we should replace these with the C++ equivalent -- stringstream.

C++ reference for stringstream: http://www.cplusplus.com/reference/sstream/stringstream/stringstream/

[Quintus]

Is this still relevant?

Vale, Quintus

[datahead8888]

Is this still relevant?

These calls are still in the code, yes. If we want to use a C++ standard, this task cannot be closed.

[Luiji]

@datahead8888 Using sprintf is fully within the bounds of the C++ standard. However, consistently with our use of C++ templates would be correct.

Additionally, sprintf is insecure because it has no buffer overflow checks which is not a problem with stringstream which dynamically resizes its internal buffer.

EDIT: If Quintus agrees I'll go ahead, grep through the code and replace all instances of sprintf.

[datahead8888]

We had made a decision to replace printf with cout / cerr where possible, with the viewpoint that it is more C++ like.

I didn't mean that sprint / printf cannot be used in C++ legitimately. I just meant stringstream would be more consistent with our interpretation of leveraging C++.

This task isn't urgent - I just filed it so that we would have a record of it.

[Quintus]

sprintf() may have its security flaws, but there are some formatting cases it handles really good you have a hard time coding by hand with stringstream or whatever. However, we don’t have such in the codebase of TSC I guess, so replacing it with stringstream or other means is apropriate. If you find yourself you have to create 50+ lines just to replace a sprintf() call, then you want to keep sprintf().

Valete, Quintus

[Luiji]

@Quintus In such instances where sprintf formatting is necessary it is much more secure to use snprintf which maintains buffer overflow checks. I'd be fine with replacing it that way as well, since normally stringstream-based checks take up more code. Additionally, I realize now that even the example @datahead8888 provides would be much simpler using a printf formatters.

I would like to propose that we change our decision to instead replace all sprintf calls with snprintf calls.

[datahead8888]

I'd be fine with replacing it that way as well, since normally stringstream -based checks take up more code.

cout usually takes a little more code than printf as well (though it's not quite as bad as stringstream is). My thought is that we might consider going ahead and using stringstream to be consistent unless it really bloats the code.

snprintf would be better than sprintf, however.

[Luiji]

@datahead8888 Consistency shouldn't mean bloating the code with unnecessary constructs. The less code there is the more maintainable it will be overall.

[Quintus]

I would like to propose that we change our decision to instead replace all sprintf calls with snprintf calls.

Is snprintf() properly standardised so that we can rely on it even being available in Microsoft’s Virtually broken C compiler (MSVC)?

If so, I don’t see a problem in using it.

Vale, Quintus

[datahead8888]

I'm fine with what you think is best on this.

[Luiji]

@Quintus I...I was about to say "yes, of course!" but apparently I've overestimated Microsoft. Instead, we're stuck with their printf_s and _snprintf (note: _snprintf != snprintf because screw you). Apparently snprintf is C++0x standard.

It may be worth just sticking with sprintf because of this, unless you want to add a dependency on Boost.Format or a custom implementation of sprintf, or are willing to go off standard and depend on snprintf when available and use a stub function on MSVC like this:

#ifdef _MSC_VER

#define snprintf c99_snprintf

inline int c99_snprintf(char* str, size_t size, const char* format, ...)
{
    int count;
    va_list ap;

    va_start(ap, format);
    count = c99_vsnprintf(str, size, format, ap);
    va_end(ap);

    return count;
}

inline int c99_vsnprintf(char* str, size_t size, const char* format, va_list ap)
{
    int count = -1;

    if (size != 0)
        count = _vsnprintf_s(str, size, _TRUNCATE, format, ap);
    if (count == -1)
        count = _vscprintf(format, ap);

    return count;
}

#endif // _MSC_VER

(Courtesy of https://stackoverflow.com/questions/2915672/snprintf-and-visual-studio-2010)

In the end, it may be worth just putting this on the back burner until C++0x finds it's way into MSVC or we deem MSVC non-priority.

[Quintus]

Also, one thing to consider with regard to internationalisation. Gettext highly recommends the use of format strings (see the manual) so that translators can move the formatting directives around to better fit their language. For any content passed to Gettext via the _() macro family, using sprintf() or one of the similar functions is required.

unless you want to add a dependency on Boost.Format

No, definitely I do not want yet another dependency.

use a stub function on MSVC like this:

This seems to be the best solution, but I wouldn’t test for the MSVC macro like this. Once MSVC supports snprintf(), it would clash. It is more useful to do it like this:

#if defined(_MSC_VER) && _MSC_VER <= 0xwhateverversioniscurrent
//...
#endif

Vale, Quintus

[datahead8888]

Honestly, if we have to write logic to compile differently for Visual C++ in order to implement snprintf, I think we should just close this ticket as "Won't Implement". That is even more complicated than stringstream, which at least is used consistently by different compilers and accepted by the community.

Yes, I think we should eventually support Visual C++ on some level with documentation on how to get TSC to work in it. When I took a real time rendering class last semester, Visual C++ was the most popular compiler. Having it available for developers to use will probably increase the number of programmers who contribute. At the very least our code has to compile in it; the moment we drop compilation support for Visual C++ it may become impractical to add it back. I use Visual C++ regularly myself and have several features I like in it that as far as I know either are not supported by g++ or would probably require additional, separate tools to get.

[Luiji]

@Quintus:

Once MSVC supports snprintf(), it would clash.

Not necessarily. That code was specifically designed not to clash. It defines the real functions under a different name and then defines the macro such that it will only affect subsequent usage. It would only cause problems of snprintf is used in inline functions in headers, assuming that you included stdio.h before this definition occurs.

However, if you want to avoid that possibility as well, you can instead put in this macro:

#ifndef _MSC_VER
#define tsc_snprintf snprintf
#else
inline int tsc_snprintf(char* str, size_t size, const char* format, ...)
{
    int count;
    va_list ap;

    va_start(ap, format);
    count = tsc_vsnprintf(str, size, format, ap);
    va_end(ap);

    return count;
}

inline int tsc_vsnprintf(char* str, size_t size, const char* format, va_list ap)
{
    int count = -1;

    if (size != 0)
        count = _vsnprintf_s(str, size, _TRUNCATE, format, ap);
    if (count == -1)
        count = _vscprintf(format, ap);

    return count;
}
#endif

This may be preferable than checking _MSC_VER <= 0x... since we won't have to update it for every release that _MSC_VER increases without snprintf support.

@datahead8888: My main problem with not fixing this yet is that usage of sprintf may result in random behavior as other parts of memory are overwritten or segmentation faults. These sort of bugs are some of the most difficult to trace since the former does not raise exceptions and the later results in an immediate abort.

[Quintus]

@Luiji Looks fine to me. Pin a comment on why we need this construct above it, and commit it to the repository as a separate C++ header file, probably core/compiler-specifics.hpp or something like that, so that all the compiler-specific stuff we may also add to in the future is contained in one single file.

Once done, replacing the sprintf() calls can start.

Vale, Quintus