v2-0 branch -Wundef flag

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
Post Reply
cord
Halfling
Posts: 59
Joined: Tue Oct 22, 2013 10:22 am

v2-0 branch -Wundef flag

Post by cord » Thu Nov 07, 2013 4:35 am

dark_sylinc has done some really great work with the v2-0 branch. It's in sinbad's bitbucket under the v2-0 branch, but it's still alpha code.

So I spent the last week working on it to try to fix it up for Linux and OSX. I would like to propose adding this to the compiler flags on Linux, OSX, iOS, Android, and MinGW: (I don't know if there is an MSVC flag like it)

-Wundef

The gcc docs explain that in certain situations, macros can have unintended effects. Here's an example:

#if OGRE_PLATFORM == OGRE_PLATFORM_FLASHCC

That might seem safe, until something happens like the following (totally legal by default in gcc and clang):

OGRE_PLATFORM_FLASHCC is typically not defined at all --> compiler will convert it to 0 silently

if by some chance, OGRE_PLATFORM is not defined, say you're in a header file and the #include's at the top omit OgrePlatform.h ... then

OGRE_PLATFORM -> compiler will convert it to 0 silently

Now the line #if OGRE_PLATFORM == OGRE_PLATFORM_FLASHCC becomes true! Because #if 0 == 0 is true. (This is actually happening in the v2-0 branch and causing segfaults.)

The gcc docs explain that -Wundef will make the compiler complain instead of just silently converting to 0. I think that can help eliminate the bugs in v2-0 and be useful in the future.

I can submit a pull request if enough lead devs think this is reasonable.
0 x

scrawl
OGRE Expert User
OGRE Expert User
Posts: 1119
Joined: Sat Jan 01, 2011 7:57 pm
x 2

Re: v2-0 branch -Wundef flag

Post by scrawl » Thu Nov 07, 2013 4:40 am

Sounds good to me. We also need to fix the tons of warnings that are already triggered though. A single warning for -Wundef would definitely get lost in there :lol:
0 x

cord
Halfling
Posts: 59
Joined: Tue Oct 22, 2013 10:22 am

Re: v2-0 branch -Wundef flag

Post by cord » Thu Nov 07, 2013 4:41 am

Well I did a test compile on Linux to see what -Wundef would look like. You would notice. It spits out lots and lots of lines of warnings on every single file.
0 x

scrawl
OGRE Expert User
OGRE Expert User
Posts: 1119
Joined: Sat Jan 01, 2011 7:57 pm
x 2

Re: v2-0 branch -Wundef flag

Post by scrawl » Thu Nov 07, 2013 4:42 am

Good!
0 x

User avatar
masterfalcon
OGRE Team Member
OGRE Team Member
Posts: 4270
Joined: Sun Feb 25, 2007 4:56 am
Location: Bloomington, MN
Contact:

Re: v2-0 branch -Wundef flag

Post by masterfalcon » Thu Nov 07, 2013 5:10 am

I'm on it! I've already fixed a ton of warnings as part of my ARM work. Just haven't committed it yet. But while we're at it. Do we really need the flash support?
0 x

cord
Halfling
Posts: 59
Joined: Tue Oct 22, 2013 10:22 am

Re: v2-0 branch -Wundef flag

Post by cord » Thu Nov 07, 2013 5:26 am

masterfalcon wrote:I'm on it! I've already fixed a ton of warnings as part of my ARM work. Just haven't committed it yet. But while we're at it. Do we really need the flash support?
I like the work you're doing. Do you want my help? Do you approve of adding -Wundef?
0 x

User avatar
masterfalcon
OGRE Team Member
OGRE Team Member
Posts: 4270
Joined: Sun Feb 25, 2007 4:56 am
Location: Bloomington, MN
Contact:

Re: v2-0 branch -Wundef flag

Post by masterfalcon » Thu Nov 07, 2013 5:33 am

I like the idea of fixing any warnings that it generates however, I'm seeing a bunch from external sources like boost headers. So adding it would probably add a lot of unwanted compiler noise.
0 x

cord
Halfling
Posts: 59
Joined: Tue Oct 22, 2013 10:22 am

Re: v2-0 branch -Wundef flag

Post by cord » Thu Nov 07, 2013 5:39 am

masterfalcon wrote:I like the idea of fixing any warnings that it generates however, I'm seeing a bunch from external sources like boost headers. So adding it would probably add a lot of unwanted compiler noise.
That's a good point. I suggest turning off -Wundef if boost is used instead of libstdc++, or just for the Threading files if boost is used for threading. I agree, my intent is not to create unwanted compiler noise.

Can I help out with the other fixes you're working on?
0 x

User avatar
masterfalcon
OGRE Team Member
OGRE Team Member
Posts: 4270
Joined: Sun Feb 25, 2007 4:56 am
Location: Bloomington, MN
Contact:

Re: v2-0 branch -Wundef flag

Post by masterfalcon » Thu Nov 07, 2013 5:43 am

Actually, I am just doing a little bit of profiling and clean up before I commit it. Probably in the next day or so depending on how much time I have.
0 x

cord
Halfling
Posts: 59
Joined: Tue Oct 22, 2013 10:22 am

Re: v2-0 branch -Wundef flag

Post by cord » Thu Nov 07, 2013 6:15 am

masterfalcon wrote:Actually, I am just doing a little bit of profiling and clean up before I commit it. Probably in the next day or so depending on how much time I have.
Ok, I'll submit a -Wundef pull request on or before Nov 9, 2013. That leaves me time to test against your commit.
0 x

User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
Contact:

Re: v2-0 branch -Wundef flag

Post by Klaim » Thu Nov 07, 2013 12:38 pm

masterfalcon wrote:I like the idea of fixing any warnings that it generates however, I'm seeing a bunch from external sources like boost headers. So adding it would probably add a lot of unwanted compiler noise.

In the lasts versions of CMake you can add defines to be uses only for the library compilation, not for users of the library compilation.
Adding the compilation define to Ogre projects but in "private" mode will do this.
See http://www.cmake.org/cmake/help/v2.8.12 ... efinitions

I'm not sure if you need CMake 2.8.11 or 2.8.12 to be able to do this but I guess it should be considered only if it's ok to bump the cmake version (I see no problem in bumping any tool versions for Ogre 2.0 that will anyway change a lot of interfaces).
0 x

cord
Halfling
Posts: 59
Joined: Tue Oct 22, 2013 10:22 am

Re: v2-0 branch -Wundef flag

Post by cord » Mon Dec 02, 2013 5:19 pm

@masterfalcon:

Thanks for the comment:

"I'll fix this the proper way tonight. It's on my radar."

I get that dark_sylinc doesn't want the fix to end up hiding other errors. To me, the problem is that any code that starts with #if MACRO_NAME is broken because it can silently fail if MACRO_NAME somehow ended up not being defined yet. If the code shouldn't compile without MACRO_NAME, do:

#ifndef MACRO_NAME
#error Expected MACRO_NAME to be defined here
#endif

This is a real problem. I started working on this after I identified a segfault caused by a #if MACRO_NAME that got silently passed over by the compiler.

The CMake 2.8.12 discussion is off topic, please start a separate thread for it.
0 x

User avatar
masterfalcon
OGRE Team Member
OGRE Team Member
Posts: 4270
Joined: Sun Feb 25, 2007 4:56 am
Location: Bloomington, MN
Contact:

Re: v2-0 branch -Wundef flag

Post by masterfalcon » Mon Dec 02, 2013 9:30 pm

Thanks cord. This has already been dealt with on the 1.9 branch and will be merged into 2.0 at some point in the future.
0 x

Post Reply