Use std::cerr for printing warnings

[Quintus]

Warnings should go to standard error, not to standard output. SMC often uses printf(), which a) prints to standard output and b) is legacy C code if you don’t need to formatting functionality (in which case fprintf() should be used with the standard error stream here).

[datahead8888]

How do you think statements like this one should be output:
printf( "Initializing Audio System - Buffer %i, Frequency %i, Speaker Channels %i\n", m_audio_buffer, pPreferences->m_audio_hz, m_audio_channels );

That one's not a warning so much as a notification. Would you prefer cout or cerr here.

I'm going to see if I can knock some of these off (how many I do for now depends on how many are out there). This will be a good chance for me to try out a pull request with you.

Also, would there be any benefit to using a logging API similar to log4j? When I did business programming a few years ago, they were pretty clear they didn't want people using System.out.println in Java -- they wanted log4j for everything. It's just a thought.

Lastly, I don't think I'm realistically going to be able to test all of these myself. I can probably try redirecting the error stream output while invoking SMC on the command and see what's showing up.

[Luiji]

cerr should always be used for any type of error or warning. cout is for non-erroneous output (e.g. generated text from awk). As far as I can tell, all of SMC's output should be put to cerr, including log entries, to be more consistent with the UNIX philosophy that generated the cerr/cout/cin mechanism. (Correct me if UNIX philosophy contradicts me.)

[datahead8888]

So you would consider, "Initializing Audio System" to be a warning?
There may be an argument for having it all in one stream; I just wanted to understand the reasoning.

[datahead8888]

Also, does anyone have a problem leaving \n in the code, or are we wanting to switch to endl? It would be a lot of typing to strip the vast number of \n's out.

[Luiji]

So you would consider, "Initializing Audio System" to be a warning?

I would consider it to be debugging information, which arguable could be in either cerr or cout. Personally, when I write programs, I keep debugging information in cerr to be consistent with programs I write where cout is the output of regular operation.

Also, does anyone have a problem leaving \n in the code, or are we wanting to switch to endl? It would be a lot of typing to strip the vast number of \n's out.

I personally think it's much more consistent to use endl instead of \n. However, we could completely circumvent this if we instead vie for a logging subsystem where we run calls like debug("This is a debugging message"), warn("This is a warning"), and error("This is really only used at the top-level exception handler"), automatically ending it with a newline. Of course, this rather defeats the point of our integration into the C++-style stream, and I've personally encountered the problem where you end up using stringstream more often then you want because of it. E.g. you'd have to do:

std::stringstream ss;
ss << "stuff" << i << "want";
debug(ss); // of course, debug() has an override to auto-call ss.str() inline

But then again, now we have debugging, warning, and error calls separated into functions that can have special handling, like colorizing them or allowing users to suppress debug output to the console.

A lot of this has to do with preference. We could do something like this:

pLogger->debug << "Hello" << endl;

...as well, but last time I tried it was fairly...over-complicated.

[datahead8888]

 I would consider it to be debugging information, which arguable could be in either cerr or cout. Personally, when I write programs, I keep debugging information in cerr to be consistent with programs I write where cout is the output of regular operation.

@Luiji - What would you consider to be normal output or regular operation for SMC in this case?

 I personally think it's much more consistent to use endl instead of \n.

Yes, endl probably is better. I'll see if I can add it to the previous lines I just changed.

Here's an interesting one:

#ifdef _DEBUG
    #define debug_print(format, ...) printf(format, ##__VA_ARGS__)
#else
    #define debug_print(format, ...)
#endif

What might be a way to handle this? Another part of the code had:

if( m_debug )

...before a printf. If this is how we do things, completely finishing this task might require changing how the debugging modes are used.

Also, for any reference I find to "Turtle" in messages for the Army enemy, I'm changing it to "Army". Any reference to "Turtle Boss" I'm not changing right now, since the graphics are still for a turtle in the game.

Shall we replace sprintf calls with std::stringstream?

Also note that wherever std:string is printed, we should try to get rid of the .c_str() calls where possible. A few of them, however, might require API digging to really verify what type of data is getting printed.

[datahead8888]

I remember seeing some discussion about a new standard for #includes for standard libraries. If I need to #include iostream, where would you prefer that include to go? I seem to remember @Quintus saying he wanted all includes in one header file.

Also, in the coding standards discussion, issue #87, we will need to decide whether to use:

using namespace std;

cerr << "Tor!" << endl;

or

std::cerr << "Tor!" << std::endl;

I think the latter is verbose unless you have a lot of namespace conflicts that require it, but I'll mention this when I get a chance to reply to the coding standards discussion task.

[Quintus]

I think the latter is verbose

I’ve seen a using namespace std somewhere in SMC’s codebase. I’m fine with this, but you probably have to run some regexps on SMC’s code to strip out all existing std:: prefixes for consistency. And while we are on it, we can probably do the same with the boost namespace — or do we get conflicts then?

Regarding @Luiji’s suggestion of a logging subsystem, I think we shouldn’t go too complicated. SMC is not a daemon which I would expect to be able to log into syslog, but an end-user program. If we can get a simple implementation of pLogger, I probably wouldn’t be against it, though, as we could then introduce a --verbose switch to SMC. But note we might get into conflict with the debug_print macro @datahead8888 already showed, which adds debugging output directly at compile time, which has the neat benefit of not hitting SMC’s runtime performance by completely eliminating the outputting code as opposed to merely silencing it.

Vale,
Quintus

[datahead8888]

@Quintus, what is your view on cerr versus cout? Do you go with Luiji's philosophy of cerr for (about) everything, or do you think we should have cout for notifications and cerr for warnings/errors?

Do we want to get iostream through core/global_basic.hpp? After adding iostream to that file, I would need to # include global_basic.hpp in a lot of source files that output debugging text. Might this bloat the executable size at all?

It sounds like we're going to need more discussion on the macro and how we want to use it going forward. If we want performance gains from it, it should be used consistently throughout the code. Conditional compilation for couts is a common technique for performance intensive applications.

[Quintus]

After adding iostream to that file, I would need to # include global_basic.hpp in a lot of source files that output debugging text. Might this bloat the executable size at all?

Why should it? The linker should be smart enough to figure out what’s duplicate and doesn’t need to be included into the final execuable. Just include SMC headers wherever you need them, just external headers should only be included in global_basic.hpp so we can be sure to have the include order correct.

what is your view on cerr versus cout?

SMC’s output is not meant to be consumed by any other program. I think informative messages therefore can go to cout, and warnings and errors should go to cerr, in case someone redirects stdout to /dev/null or so.

Vale,
Quintus

[datahead8888]

 external headers should only be included in global_basic.hpp so we can be sure to have the include order correct.

iostream is external, so it's going to need to be added to the global_basic.hpp then.

 I think informative messages therefore can go to cout, and warnings and errors should go to cerr, in case someone redirects stdout to /dev/null or so.

This is in line with what I was taught about cout and cerr. I don't suppose it will be easy for people to combine them into one unified stream in the original write order (correct me if I'm wrong), but it's more important to differentiate warnings versus informative text.

[Quintus]

iostream is already in the include list: https://github.com/Quintus/SMC/blob/devel/smc/src/core/global_basic.hpp#L54

[datahead8888]

I thought I ran a text search earlier. I somehow missed it. Thanks for catching that.

[Luiji]

What would you consider to be normal output or regular operation for SMC in this case?

Well, there isn't any "normal output", it's all debugging information (including the warnings and errors). I generally come from a lot of more UNIX-y type programming where the separation between output and debugging information is important, but SMC doesn't really exist in this zone because the UNIX-y type programming system wasn't designed for video games.

In a sense, nothing should actually be written to the terminal since this is a graphical application. The only reason we do it is for debugging. As such, "The Right Way (TM)" is inapplicable here (plus, The Right Way (TM) is ITS's philosophy, not UNIX's).

So, in the end, we have to argue which strategy is more useful, not which is more congruent.

SMC’s output is not meant to be consumed by any other program. I think informative messages therefore can go to cout, and warnings and errors should go to cerr, in case someone redirects stdout to /dev/null or so.

For instance like that. That's a good feature idea: allow people to disable debugging messages by redirecting to /dev/null. However, I argue against that because such a feature should be implemented as a command-line option (--quiet).

However, might it be useful to redirect debugging information to one file and error information to another? In that situation, I'd see this separation as useful.

[Quintus]

We could also directly write everything to a logfile instead of outputting to the console. However, I find the live outputting very useful for debugging as I immediately see at which point the program crashed even without using gdb.

Vale,
Quintus