header guard and warning issues

[al2950]

I have just checked out the latest version of 1.8, mainly due to boost version issues, and a found a couple of other issues. Some of the issues may be down to me destroying my build machine but below are a couple of examples:

• Loads of warnings (nearly 50000!) in windows, almost all are C4251. I assume an include chain has been broken somewhere so OgreHeaderPrefix is not being included in the correct places. Anyway visual studio struggles with printing all those warnings and so takes forever to build!
  Ah.... Just found the issue, rev 3704 should never have been committed. There was a good reason the header guards being like that. Suggest backing out that changeset and adding a comment to OgreHeaderSuffix.h so no one makes the same mistake in the future.
• Secondly I tried to enable the profiler and it crashed straight away. I noticed some profiler improvements have been added so I wont worry about it too much, although I dont think it should be going into the stable branch.

I appreciate thats only 2 things but it does not give much confidence on our stable branch. Having said that I may have just updated at particularly bad point in time. If someone wants I can create patches of the two issues above, although it may be quicker for Ogre team member to just do it straight off Anyway let me know

[masterfalcon]

I can back out the header suffix commit. I never saw any issue here, but I'm not using windows.

As for the profiler issue, how is it crashing? Could you give more information on how you're setting everything up, compiling Ogre, what the backtrace is?

[Klaim]

Loads of warnings (nearly 50000!) in windows, almost all are C4251. I assume an include chain has been broken somewhere so OgreHeaderPrefix is not being included in the correct places. Anyway visual studio struggles with printing all those warnings and so takes forever to build!
Ah.... Just found the issue, rev 3704 should never have been committed. There was a good reason the header guards being like that. Suggest backing out that changeset and adding a comment to OgreHeaderSuffix.h so no one makes the same mistake in the future.

Can you explain what you mean by "good reasons"? I don't see any good reason at all for wrong header guards.
Also, maybe check their use: these guards were certainly hiding problems.


edit: By the way, I think the header guards were fixed correctly (I pointed the problem actually), but these headers have been thought to be used almost everywhere, and certainly aren't. Maybe some CMake magic could do something about this?

[masterfalcon]

Crap, maybe I backed it out too soon.

[al2950]

@ materfalcon
I am just debugging now, I will let you know. It may have something to do with my fairly custom render loop & nested profile groups. It did causes issues before although it never crashed.

@Klaim
I have looked at it closer now and you do have a point! Its all down to how OgreHeaderPrefix and OgreHeaderSuffix are meant to work together. Before Rev 3704, the prefix header disabled a number of warnings, however because of the incorrect header guard in the suffix, those warnings were never re-enabled, so we all lived in ignorant bliss! In my mind for it to work correctly you should have the correct header guards but make sure #undef both defines at the the suffix header so the pair can be used again. Although I just tried it and I get a lot warnings still , although a lot less.

I hope that makes sense and I am not spouting rubbish (its getting a bit late here)!

**EDIT. Warning count is still large, still building!

[al2950]

@masterfalcon
Sorry I messed up my builds so cant get debug symbols of Ogre. I will have another look tomorrow., but I can tell you it was crashing when an OgreProfileGroup went out of scope. The line it crashed on was Ln:693 in OgreProfiler.cpp (that could be wrong as its from memory).

My Ogre CMake build settings are:
Unity build
Boost 1.50
Profiler
VS 2010 32bit

**EDIT** It works fine if I enable it on startup, however if I enable it from a key press, ie half way through a frame it crashes as soon as the first OgreProfileGroup goes out of scope

[Klaim]

I think the UnityBuild might greatly impact this problem...

[al2950]

I think the UnityBuild might greatly impact this problem...

I thought that was a very good point, however I tried it and it didnt seem to improve! I am not sure how to go about this issue. Your fix for the header guards was correct, however that number of warnings is not acceptable! We could just disable those warnings project wide (ugly solution), or go through all the code and add the header prefix & suffix to the appropriate headers where they are missing (painful solution!).

[al2950]

@masterfalcon

As promised here is a callstack of when the profiler causes a crash when its enabled half way through an update loop. When I say update loop, I mean my internal update loop, not when Ogre is rendering!;

ProfilerCrash.png

However more worryingly I found that it crashes when you exit, I had a quick look at the code and it crashes in the destructor of ProfilerInstance because the iterator becomes invalid due to it being edited (erasing an element) as its iterating through. This is bad C++ code and I am suprised it made it into the Ogre repo

[Klaim]

I think the UnityBuild might greatly impact this problem...

I thought that was a very good point, however I tried it and it didnt seem to improve! I am not sure how to go about this issue. Your fix for the header guards was correct, however that number of warnings is not acceptable! We could just disable those warnings project wide (ugly solution), or go through all the code and add the header prefix & suffix to the appropriate headers where they are missing (painful solution!).

Ok let me help you: I cannot try yet the lest Ogre 1.8 version, but I know have a similar warning problem with my current project.

A very simple way to fix this is to add:

Code: Select all

if( MSVC )
 # Deactivate warning C4251
 set( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4251" )
endif()

somewhere in the CMake files, maybe the root one is the best place, or maybe in one of the project definition macros.

It work in my project (with MSVC) so it should work for you too.
However, this fix only one warning. You can add the others that are deactivated in the header.

I think this kind of stuffs should definitively be moved in CMake. That said it should (I think) trigger the same warning in client code of Ogre. I'm not sure though.
That said, if using Ogre, the client code still have no other choice than using STL in the interface so it should be ok let the client code disable the warnings too.

[Wolfmanfx]

Yeah i also noticed it - most warnings regards the wrong usage of OGRE_EXPORT / OGRE_PRIVATE macros (dll export).

[Klaim]

I upgraded to VS2012 monday and I remark this warning in my cpp files including some Ogre headers:

Code: Select all

1>...\ogre\ogremain\include\OgreHeaderSuffix.h(34): warning C4193: #pragma warning(pop) : no matching '#pragma warning(push)'
1>...\ogre\ogremain\include\OgrePixelFormat.h(197): warning C4275: non dll-interface struct 'Ogre::Box' used as base for dll-interface class 'Ogre::PixelBox'
1>...\ogre\ogremain\include\OgreCommon.h(661) : see declaration of 'Ogre::Box'
1>...\ogre\ogremain\include\OgrePixelFormat.h(197) : see declaration of 'Ogre::PixelBox'

It's Ogre 1.8.1 (plus a patch).

Just for you to know that now the problem I pointed in http://www.ogre3d.org/forums/viewtopic.php?f=22&t=71238 is visible to VS2012 users...

Nothing to hurry though, just wanted to keep you informed.

[FlorianGeorge]

Code: Select all

1>...\ogre\ogremain\include\OgreHeaderSuffix.h(34): warning C4193: #pragma warning(pop) : no matching '#pragma warning(push)'

Yeah that one's a little nuisance.

Posted a Mantis Issue here: http://www.ogre3d.org/mantis/view.php?id=559

Using VS2008SP1, when compiling Ogre 1.8.1 as well as compiling applications using Ogre 1.8.1, I am now getting a wall of text critting hard with this sentence:

OgreHeaderSuffix.h(34): warning C4193: #pragma warning(pop) : no matching '#pragma warning(push)'

I fixed it for me by changing the lines

// restore previous warnings settings
# pragma warning (pop)

to

// restore previous warnings settings
#pragma warning( disable : 4193 )
# pragma warning (pop)
#pragma warning( disable : 4193 )

Not sure if this fix is clean enough to be added to the repository, but for me receiving those warnings cluttered up my build output too much to just let them slide.

[Antodologo]

I think the "warning C4193: #pragma warning(pop) : no matching '#pragma warning(push)'" bug is because of the problem I pointed here: http://www.ogre3d.org/forums/viewtopic.php?f=22&t=71238

There is an undef missing in "OgreHeaderPrefix.h":

Code: Select all

#undef __OgreHeaderPrefix_H__

Without that undef, push is called only once and pop is called always (because "OgreHeaderSuffix.h" has the undef) and that creates the warning when you try to set new pragma warnings before a pop without push.

Could you check it out and add it to the mantis bug if you agree with me?


Anyway I think I will break my OgreHeaderPrefix/Suffix code to avoid all that annoying warnings as it was before, but bugs should be solved in the repository

[Wolfmanfx]

So i removed the OgreHeaderPrefix/Suffix file completely in 1.9 and disabled the warnings via cmake which do it on project level and not global - if you guys have a better solution i am open to it

[Klaim]

You disabled the same warnings than in the file right?

If you did it in a way that apply to Ogre projects only, then the problem remains because non-Ogre projects will compile Ogre headers with these specific warnings...
If you managed to avoid that, then it seems perfect to me.

[Wolfmanfx]

Yes i disabled the same warnings than before.
Yeah i applied it just for OGRE projects.
I thought about and maybe i just think the wrong way but i do not see any benefits to clutter the whole ogre codebase with the prefix/suffix header to disable MSVC specific warnings (we have also clang, gcc).
Also for a MSVC user its easy to disable these warnings

warn.jpg

the way it worked before was just that it was disabled globally (by accident) and maybe its better to leave the choice to the user how he wants to disable these warnings when he includes OGRE but then again if we have a better solution where do not need to clutter our header files i am open to it.

[Antodologo]

If the idea is to disable those warnings forever I prefer doing it in the code rather than in the project, making every dependent project disable the same warnings and without possible improvements with a future Ogre version.

I have commented out the push/pop in OgreHeaderPrefix/Suffix for a similar effect (undefs are not needed for this solution)

Code: Select all

#   pragma warning (push)

Code: Select all

#   pragma warning (pop)

The warnings are then disabled in the code, not a smart solution, but I haven't got a better one for my purposes (and it allows improvements with new code in future versions or changes in the warning codes involved)

[Wolfmanfx]

@Antodologo
Yeah you just disabled the warnings globally - i disabled it just for the ogre project because it creates to much noise when i compile ogre with msvc so real warnings get lost due the noise.
Sinbad made nice comments why he disabled these warnings in the first place or better why we can ignore them inside OGRE.

[Klaim]

The thing is some (if not all) of these warnings are useful for the client code. For example i want to see those warnings but one in my case. I deactivated it the same way than you in CMake (the suggestion I made before) but it is located in my own projects. The other warnings are useful to me, but I don't want Ogre to pollute my code's warnings (though I'm used to it now, because I use a lot of dependencies...).

So the problem is that, before, the headers were wrong leading to hiding warnings; but now the config don't hide warning for client projects but also show them exclusively in client code compiling with Ogre headers.

The perfect solution would be one injecting the pragmas automatically before (deactivating) and after (activating) any Ogre header.
However I think it is just not possible (have to be checked) and getting the headers back would be better...

Do someone see another possible solution?

[syedhs]

I don't know whether this will achieve what we want, but one way is to define a macro which you set to 1 (or simply define it) in Ogre project. And if this macro is defined or equal to 1, then all those pragma warnings are executed.

[Klaim]

Doesn't it stays the same problem? Where do you put that macro? If it's in a header, it's still necessary to either include it in user code or Ogre code (depending on what you meant exactly).
User code shouldn't have to be changed because of Ogre's internal setup.

[CoreDumped]

Doesn't it stays the same problem? Where do you put that macro? If it's in a header, it's still necessary to either include it in user code or Ogre code (depending on what you meant exactly).
User code shouldn't have to be changed because of Ogre's internal setup.

I think what syedhs was saying is we have a preprocessor flag set by CMake for each OGRE project (e.g. OGRE_INTERNAL)

Then in the OgreStableHeaders.h file (and only that file), we have something like this

Code: Select all

#ifdef OGRE_INTERNAL
#include "OgreHeaderPrefix.h"
#endif

// ... OgreStableHeaders.h code

#ifdef OGRE_INTERNAL
#include "OgreHeaderSuffix.h"
#endif

This way every ogre project has the warning disabled

[Klaim]

That shouldn't fix anything as the problem is both in Ogre code and client code: the point is to make sure client code isn't impacted by warning from ogre headers too.
Also what you describe cannot work either for Ogre code as it still required the suffix include to be included at the end of the code, not in a header.

I think this cannot be fixed without checking all Ogre source files. Maybe some script to check the code and fix it or at least point where there is missing prefix/suffix headers.

[oiking]

What about removing the include guards for the Prefix/Suffix headers completely?

The #pragma warning (push / pop) manipulates a stack, anyway, so it should build up and tear down correctly when the headers are used correctly.