Compiler errors when using a strict compiler flags

[GoogleCodeExporter]

node.cpp
m_pIdentity(this) should be disallowed in initializer list in both 
constructors. Use it in the constructor body instead.

Scanner::GetStartTokenFor
-> Not all paths return a value.

Tag::Translate
-> Not all paths return a value, missing return from default case.

Version 0.2.5

I'm willing to upload these changes.

Original issue reported on code.google.com by nite...@google.com on 17 Nov 2010 at 1:32

[GoogleCodeExporter]

What would you suggest doing in the last two cases? The only way those cases 
could happen is by not passing a valid input. I think there's no reason to do 
anything other than assert.

As for the first one, I think that's an overly stingy compiler. They're just 
trying to prevent you from accessing member variables before the object is 
fully constructed. But we do - all passing 'this' does is hand over the 
pointer. So I recommend just turning that warning off.

Original comment by jbe...@gmail.com on 17 Nov 2010 at 2:55

• Changed state: WontFix

[GoogleCodeExporter]

For the last two cases:

Your function signature says that you return a Token::TYPE enum. However there 
is a control path that doesn't do this. Therefore an undefined value is going 
to be returned (whatever happens to be in the return register) and thus your 
program can have undefined behavior.

Therefore the solution is to just return anything - it may not be the correct 
behavior but at least it is not undefined behavior.

Same goes for the missing string return. Fix it by just returning an empty 
string.

As for just turning off the warnings:
I strongly urge you to reconsider - companies use the strictest of compiler 
settings for building their project. They will consider turning off some 
compiler options if the lib is very well tested, say zlib or freetype. But 
yaml-cpp is much smaller. Often a company will just include your source as a 
compile step. Changing the compiler settings for one lib is a very serious red 
flag. Management can either take the step of allowing the source code to be 
altered (in my case) or just forbid the code from being included at all. As of 
now, we can't include your library in the production build of the product.

So please, for the sake of yaml-cpp, fix the compiler errors.

Original comment by nite...@google.com on 17 Nov 2010 at 4:46

[GoogleCodeExporter]

You're right, I suppose I'll fix these warnings.

Here's the problem with adding the control path: it's still undefined behavior, 
since the only way you can get to that spot is by switching on an uninitialized 
variable, or casting an illegal value to that enum. In that case, it's 
undefined what happens.

I really hate that particular warning, and I don't think it should exist. I 
think the right thing to do is just to throw an exception. (I'm pretty sure the 
warning goes away if that happens, since the control path can't return.)

Original comment by jbe...@gmail.com on 17 Nov 2010 at 6:46

• Changed state: Accepted

[GoogleCodeExporter]

Right. Exception handling would unwind the stack and the return value would be 
meaningless in this case, which is exactly what you want. Sadly I work with a 
few platforms where exception handling is disabled. :(

In these cases the program just crashes when the exception is thrown, I think 
this would be preferable to undefined behavior though.

Original comment by nite...@google.com on 17 Nov 2010 at 9:22

[GoogleCodeExporter]

Okay just an update. Exceptions are obviously the right way to do this. After 
playing with the code for a day and testing it seems that the major mobile 
platform that previously lacked exception support now handles them correctly. 
So every mobile platform that we use correctly has exception support. Just 
thought you'd like to know.

Original comment by nite...@google.com on 19 Nov 2010 at 6:19

[GoogleCodeExporter]

Thanks, nite. I've changes those few spots in r437. Sorry about the wait!

Original comment by jbe...@gmail.com on 2 Mar 2011 at 5:30

• Changed state: Fixed