Use std::cerr for printing warnings about tsc HOT 36 CLOSED

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.

from tsc.

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.)

from tsc.

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.

from tsc.

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.

from tsc.

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.

from tsc.

 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.

from tsc.

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.

from tsc.

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

from tsc.

@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.

from tsc.

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

from tsc.

 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.

from tsc.

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

from tsc.

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

from tsc.

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.

from tsc.

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

from tsc.

Should we perhaps vary behavior between development and production versions?

from tsc.

If we write everything to a log file, a logging api that can output to the log file and/or console interchangeably would be appropriate -- it would then be configurable. I'm not saying we should use a logging api necessarily, just evaluating options there.

As for the debug_print macro, we should change it to use cout / cerr (either passing the cout / cerr object or creating two separate macros) and not printf as part of this task. A separate Github task should probably be created to make the rest of the code either use debug_print more consistently or get rid of it. We could also consider the development / production behavior as part of that task.

from tsc.

Update - I have about 7 files left with printf's, counting the one with the debugging macro.
There are some additional ones with sprintf that could be replaced with sstream.
I will need to find a reg ex tool to try and repace cout with std::cout as well as the other std and/or boost items. I will try to do it in QT Creator otherwise look for a confirm option in sed.
I still need to come up with a solution for the debugging macro, since it takes a formatting string as input. cout and cerr don't just work with formatting strings as far as I know.

from tsc.

I still need to come up with a solution for the debugging macro, since it takes a formatting string as input. cout and cerr don't just work with formatting strings as far as I know.

Indeed they don’t; C++ recommends the use of C’s printf() and sprintf() if you need formatting capabilities that exceed the features of streams (e.g. padding with zeroes or so), which is a problem I overlooked until now. Are there such complex invocations of formatting in SMC?

I will try to do it in QT Creator otherwise look for a confirm option in sed.

Emacs can do this ;-). However, for complex regexp replaces people usually recommend awk(1), which I have not yet looked into.

Vale,
Quintus

from tsc.

 Indeed they don’t; C++ recommends the use of C’s printf() and sprintf() if you need formatting capabilities that exceed the features of streams (e.g. padding with zeroes or so), which is a problem I overlooked until now. Are there such complex invocations of formatting in SMC?

I haven't looked deeply into this yet. My intuition was that some issues may arise in getting cout and cerr to work with this macro because << and >> may not work very well with a macro call when several variables need to be printed. Aside from that, I was thinking about making cout/cerr be one of the arguments to the macro. In the long term we also need to look more into the settings for debug logging. That probably belongs in a separate task from this one, though.

 Emacs can do this ;-). However, for complex regexp replaces people usually recommend awk(1), which I have not yet looked into.

I know that vi has a confirm option alongside the global replace option for %s string replaces. This is what I meant by a confirm option -- I want it to prompt me for each of the changes individually so I can more easily check them. It has to work across all files in the project, though, all at once.

from tsc.

I want it to prompt me for each of the changes individually so I can more easily check them. It has to work across all files in the project, though, all at once.

Which is exactly how Emacs handles it :-).

Vale,
Quintus

from tsc.

Update / Question:
I have knocked out all printf's except for the one in the debug macro. Now it's time to decide how to handle that.

global_basic.hpp:

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

Here's an example of it being called, from audio.cpp:

debug_print("Could not play sound file : %s\n", path_to_utf8(filename).c_str());

Quintus said:

 Indeed they don’t; C++ recommends the use of C’s printf() and sprintf() if you need formatting capabilities that exceed the features of streams (e.g. padding with zeroes or so), which is a problem I overlooked until now. Are there such complex invocations of formatting in SMC?

I looked through the usage of debug_printf, and there was nothing complicated like padding zeroes for any of the calls. The one nontrivial use is in the macro definition itself -- if you look above it has a ## in the definition. It is used 47 times, counting the macro definition as a use.

The macro definition:

• Is inherently built around using formatting strings and doesn't jive well with cout/cerr.
• Has no notion of error stream versus output stream (cout/cerr)
• Is inconsistent because some code (in Quintus's fork, not mine) uses printf while some of it uses this macro.
• Offers a debugging mode that prints differently
• Does offer possible performance benefits if we were to modify it.

One possible solution is to replace all macro calls with cout/cerr which would force it into our paradigm. I would really rather not replace all the cout/cerr calls I already typed with calls to the macro -- besides not working well with cout/cerr, it means the previous changes I did are worthless.

The other possibility is to try to find a way to get cout/cerr to work in the macro. I'm not sure how to do this -- does anyone have good ideas? We probably still won't get the benefits of cout/cerr without modifying all of the calls to the macro.

I'll still need to look into the global text replace for the using namespaces we discussed -- it's probably going to be a lot of files. Maybe std::cout would have been easier for now...

from tsc.

html logs of chat discussion with Quintus on this:
http://secretmaryo.quintilianus.eu/htmllogs/2014-07-31.log.html#msg-2014-07-31T16:48:50+02:00

from tsc.

Update:

• Made the macro use fprintf with stderr as discussed with Quintus
• Replaced std::cerr and std::cout with using namespace std equivalent
• Still need to look into boost namespace changes. There may be a couple other std:: things that can be changed as well (ie std::string).

Question - I think I #include'd global_basic.hpp in a number of files where it may not have been needed for the using namespace changes. Will this cause any harm? The only reasonable way to undo this is to revert the commit and recode the using namespace changes.

Also, I noticed that my commits added smc/CMakeLists.txt.user to github. What's the best way to purge this out of github?

from tsc.

Question - I think I #include'd global_basic.hpp in a number of files where it may not have been needed for the using namespace changes. Will this cause any harm?

I don’t think so. Just try compiling it, and you will see if you get any problems.

Also, I noticed that my commits added smc/CMakeLists.txt.user to github. What's the best way to purge this out of github?

Delete the file, commit that, and push the commit to GitHub?

Vale,
Quintus

from tsc.

 Which is exactly how Emacs handles it

How do you do a replace across all files, confirming or rejecting each change, in emacs?

from tsc.

@datahead8888

1. M-x find-name-dired, entering a start directory and then a shell glob pattern for the files you want (e.g. *.cpp).
2. t to toggle the mark for all files
3. Q for regexp replace; you are asked for a regexp now
4. Now you are asked for every single occurence if you want to replace it.

The files are not automatically saved after the replace finishes. Use M-x save-some-buffers if you want to save them all at once.

Vale,
Quintus

from tsc.

I got through getting it to use Q to do a regexp. I can't figure out how to get it to show me each occurrence in the files and confirm/reject each occurrence -- it seems to have done a mass replace automatically. I also am not sure how to switch between individual source files and the file listing where I mark files.

from tsc.

it seems to have done a mass replace automatically. I also am not sure how to switch between individual source files and the file listing where I mark files.

This sounds as if your regexp did not match anything. That method definitely does not do anything automatically for you. It will switch to each affected file and ask you. If it doesn’t your regexp did not match anything. Try with a simpler regexp for experimenting before you get to the real stuff.

Vale,
Quintus

from tsc.

I've knocked off the remaining std::'s that didn't seem to cause errors when removed. I was getting close anyways, so I went ahead and used other tools to do it. I probably need to learn the emacs technique in order to take something on like this in the future, though.

There are 261 occurrences of boost:: in the project.

The project frequently defines this:

namespace fs = boost::filesystem;

There are 270 occurrences of fs::

If it's important, I can look into changing boost:: as well, but I think it will benefit the project more for me to move on to another task once the existing std:: and cout/cerr changes are polished and then move on to another task that teaches me more of the architecture. I can create a task for all desired follow up.

I was concerned I may have accidentally used tabs based on the alignment in github diffs. I'm not seeing them in QT Creator, though, where I'm checking at the moment. I'll double check some more and may have to look into regular expression solutions if it's a problem.

Thanks for your tips, Quintus.

from tsc.

If it's important, I can look into changing boost:: as well,

#28

but I think it will benefit the project more for me to move on to another task once the existing std:: and cout/cerr changes are polished and then move on to another task that teaches me more of the architecture

Probably. Feel free to pick something from the tracker and solve it; all PRs are gladly accepted.

I was concerned I may have accidentally used tabs based on the alignment in github diffs.

You see, tabs are hell. Once the 2.0.0 final is out, there will probably be a large commit that changes all the tabs to spaces.

Vale,
Quintus

from tsc.

 You see, tabs are hell. Once the 2.0.0 final is out, there will probably be a large commit that changes all the tabs to spaces.

I looked at some examples. It seems QT Creator (my IDE) used spaces for all tabbing. Some of the surrounding code already had tabs, which don't line up with the spaces. Since the new lines of code are following the standard, my thought is to leave it and do a mass tab clean up later. We will have to get rid of tabs as you said. I'd be curious if github has a configurable tab width, though.

from tsc.

I'd be curious if github has a configurable tab width, though.

It hasn’t.

Vale,
Quintus

from tsc.

Update:

• Submitted PR #135. Quintus and carstene reviewed it.
• Based on feedback from the team, I decided to back out the changes that remove std:: from everything in the project. I am keeping the changes to change std::cerr to cerr, std::cout to cout, and std::endl to endl. My reversions seem to have stripped out my std::endl changes, so I redid those.

Tasks remaining:

• Check more diffs for the reversions
• Remove editor backup files from git and create a gitignore file.
• Merge and remove ::std from the newest couts/cerrs

from tsc.

As discussed in the PR I merged against simpletoon's packaging changes, reconciled the new printf's and std::cerr/std::cout with the direction taken in my changes, and added an entry to the gitignore file.

devel is broken right now - Quintus said it does not compile. The world map editor was broken in my fork and the fix was in release 2.0. I have now merged that branch into my fork. If we move my fork to devel, devel should be fixed and up to date.

from tsc.

Quintus merged this into the project's devel branch. This issue is ready to be closed, pending anyone else's confirmation that is needed.

from tsc.