[patch] Improve build flags and code quality
- Svenstaro
- Greenskin
- Posts: 115
- Joined: Fri Dec 15, 2006 1:30 pm
- Location: Germany
- x 3
- Contact:
[patch] Improve build flags and code quality
Hi,
I noticed Ogre doesn't use flags like -Wall, -Wextra, -Werror, -pedantic (or their MSVS equivalents). Is there any reason for that? Obviously those flags are annoying like that because you will get a lot of false warnings but those can easily be turned of using -Wno-unused-parameter -Wno-unused-but-set-parameter. In fact, Ogre used to (1.7) compile with the flags mentioned above but it no longer does (1.8rc1) and I see this is a result of not using these parameters in the first place. Using them would improve code quality due to being more compatible with projects that use strict build flags. Those projects would not compile if the compiler includes an Ogre file that does not compile using strict flags.
This is sadly an actual problem to me since my projects stopped compiling with 1.8rc1 and I'd like to improve that situation for the next rc/final.
With that in mind, I'd like to request for Ogre to adopt stricter build flags and I'm making this thread as this will likely require a discussion. On a side note, Ogre 1.8rc1 will currently not work with projects that use -std=c++0x due to this other error.
I noticed Ogre doesn't use flags like -Wall, -Wextra, -Werror, -pedantic (or their MSVS equivalents). Is there any reason for that? Obviously those flags are annoying like that because you will get a lot of false warnings but those can easily be turned of using -Wno-unused-parameter -Wno-unused-but-set-parameter. In fact, Ogre used to (1.7) compile with the flags mentioned above but it no longer does (1.8rc1) and I see this is a result of not using these parameters in the first place. Using them would improve code quality due to being more compatible with projects that use strict build flags. Those projects would not compile if the compiler includes an Ogre file that does not compile using strict flags.
This is sadly an actual problem to me since my projects stopped compiling with 1.8rc1 and I'd like to improve that situation for the next rc/final.
With that in mind, I'd like to request for Ogre to adopt stricter build flags and I'm making this thread as this will likely require a discussion. On a side note, Ogre 1.8rc1 will currently not work with projects that use -std=c++0x due to this other error.
Last edited by Svenstaro on Tue Nov 29, 2011 11:46 pm, edited 1 time in total.
-
- OGRE Retired Team Member
- Posts: 2903
- Joined: Thu Jan 18, 2007 2:48 pm
- x 58
- Contact:
Re: Improve build flags and code quality
In my opinion, the ratio of false to useful warnings with the -Wall flag is totally off and therefore it's a pain in the ass more than it's useful. As such, I'm not a fan of the -Wall -Werror combo as a default.
However, we do (and did in the past) accept patches to shut off warnings in Ogre's headers so that they don't interfere with other projects.
However, we do (and did in the past) accept patches to shut off warnings in Ogre's headers so that they don't interfere with other projects.
- Svenstaro
- Greenskin
- Posts: 115
- Joined: Fri Dec 15, 2006 1:30 pm
- Location: Germany
- x 3
- Contact:
Re: Improve build flags and code quality
Actually with -Wall -Wextra -Werror -Wno-unused-parameter -Wno-unused-but-set-parameter -pedantic a make should run through without false positives and you would know your code is super clean. I will contribute a trivial patch for the current cleanups necessary to make those flags possible.
However, a more problematic flag right now is -std=c++0x and it has something to do with Ogre's custom STL memory allocators but I can't quite deduce the error message. If you can fix that I will deliver the rest.
However, a more problematic flag right now is -std=c++0x and it has something to do with Ogre's custom STL memory allocators but I can't quite deduce the error message. If you can fix that I will deliver the rest.
-
- Bugbear
- Posts: 812
- Joined: Thu Dec 09, 2004 2:51 am
- x 42
Re: Improve build flags and code quality
From your error it looks like it expects a construct taking a single parameter in OgreMemorySTLAllocator.h , something like:
You would probably be better waiting for the developers to confirm. And off-topic in the 1.8 branch OgreImage.cpp needs to include "OgreMath.h" to build.
Code: Select all
void construct(pointer p)
{
// call placement new
new(static_cast<void*>(p)) T();
}
- Svenstaro
- Greenskin
- Posts: 115
- Joined: Fri Dec 15, 2006 1:30 pm
- Location: Germany
- x 3
- Contact:
Re: Improve build flags and code quality
I was mistaken. Compiling ogre with std=c++0x always failed and the Ogre::STLAllocator was not changed since a long time. However, ogre uses long long which is only standard since c++0x, so we'll need to make this work.
- Klaim
- Old One
- Posts: 2565
- Joined: Sun Sep 11, 2005 1:04 am
- Location: Paris, France
- x 56
- Contact:
Re: Improve build flags and code quality
Now that you're talking about it, is it a problem to have ogre compiled without C++0x and your application using c++0x? I'm guessing that no as far as you don't rely on binary compatibility, but if you did try that any feedback would be welcome.
- sparkprime
- Ogre Magi
- Posts: 1137
- Joined: Mon May 07, 2007 3:43 am
- Location: Ossining, New York
- x 13
- Contact:
Re: Improve build flags and code quality
-Werror is a dumb idea tbh -- knowledge is power, but obstacles are a pain in the arse
-Wno-long-long (possibly misspelt) may help
You shouldn't actually need -std=c++0x for trivial things like long long and additional API functions like strtoll. It will only encourage you to start using language features that are not yet mainstream. Better to start with the old standard and then 'add' the things you need explicitly, so it's clear how much of c++0x is actually needed.
Of the false warnings in -Wall -Wextra, all of them can be avoided by making trivial changes in the source code to indicate that 'actually I did want to do that'. E.g. (void)ing unused params. That way you are avoiding potential bugs *and* avoiding getting a screenful of crap everytime you compile.
-Wno-long-long (possibly misspelt) may help
You shouldn't actually need -std=c++0x for trivial things like long long and additional API functions like strtoll. It will only encourage you to start using language features that are not yet mainstream. Better to start with the old standard and then 'add' the things you need explicitly, so it's clear how much of c++0x is actually needed.
Of the false warnings in -Wall -Wextra, all of them can be avoided by making trivial changes in the source code to indicate that 'actually I did want to do that'. E.g. (void)ing unused params. That way you are avoiding potential bugs *and* avoiding getting a screenful of crap everytime you compile.
- altren
- Gnome
- Posts: 329
- Joined: Tue Oct 24, 2006 9:02 am
- Location: Moscow, Russa
- x 24
- Contact:
Re: Improve build flags and code quality
While many developers dislike this flags due to lots of not so useful warnings it is still good to use them. Main problem is that you can't just turn it on for some big projects, because you'll get way too much to handle. I spent few days, when I decided to do that with MyGUI. We ended with several gcc flags (-Wall is far from all useful warnings) and some disable warnings like unused parameter. Here's list of flags we use (and having 0 warnings in current MyGUI code, also there were almost no useless changes just to make it shut up):
(use gcc documentation to figure what does this warnings are used for).
Also it is important to make 3rd party libraries not produce warnings, since they are included, but you probably not going to change them:
And last thing that might help in such task is next flag - it show which flag generate warning, so if you dislike warning, but have no idea which flag to use this diagnostic message will help:
Code: Select all
-Wno-deprecated -Wall -Wctor-dtor-privacy -Winit-self -Woverloaded-virtual -Wcast-qual -Wwrite-strings -Wextra -Wno-unused-parameter -pedantic -Wshadow
Also it is important to make 3rd party libraries not produce warnings, since they are included, but you probably not going to change them:
Code: Select all
-isystem ${OGRE_INCLUDE_DIR}
-isystem ${OIS_INCLUDE_DIR}
Code: Select all
-fdiagnostics-show-option
This option instructs the diagnostic machinery to add text to each diagnostic emitted, which indicates which command line option directly controls that diagnostic, when such an option is known to the diagnostic machinery.
Use -Wno-unused-parameter with -Wall. If you think, that some warnings are not really useful - just disable them, but keep other useful warnings.sparkprime wrote:E.g. (void)ing unused params. That way you are avoiding potential bugs *and* avoiding getting a screenful of crap everytime you compile.
- altren
- Gnome
- Posts: 329
- Joined: Tue Oct 24, 2006 9:02 am
- Location: Moscow, Russa
- x 24
- Contact:
Re: Improve build flags and code quality
Ah, and one more thing - since we should not bother library users with warnings we use additional CMake option, that turn off extra warnings with gcc or msvc (W3 instead of W4).
- sparkprime
- Ogre Magi
- Posts: 1137
- Joined: Mon May 07, 2007 3:43 am
- Location: Ossining, New York
- x 13
- Contact:
Re: Improve build flags and code quality
useful stuff you postedaltren wrote: ...
altren wrote:Use -Wno-unused-parameter with -Wall. If you think, that some warnings are not really useful - just disable them, but keep other useful warnings.sparkprime wrote:E.g. (void)ing unused params. That way you are avoiding potential bugs *and* avoiding getting a screenful of crap everytime you compile.
But then you don't avoid bugs, you are disabling the warning when it is helpful as well as when it is not helpful.
The point is that you get the warning and then you say "that's ok i meant to do that", make a minor modification in the code to signal your intent and make the warning go away, but when perhaps you have a real bug like void test (int x, int y) { return x == x; } the warning will still help you pick it up
- Svenstaro
- Greenskin
- Posts: 115
- Joined: Fri Dec 15, 2006 1:30 pm
- Location: Germany
- x 3
- Contact:
Re: Improve build flags and code quality
Here, I made some changes to improve code quality. It's not complete but it's a start. Also, we definitely need that STLAllocator changes in or compiling against projects that use std=c++0x will fail. See my cleanups branch here: https://bitbucket.org/svenstaro/ogre
Patch entry: https://sourceforge.net/tracker/?func=d ... tid=302997
Patch entry: https://sourceforge.net/tracker/?func=d ... tid=302997