Since it recently made news I thought it would be interesting to run cppcheck(v1.61) over FO.
Overall it looks pretty good:
freeorion_0.4.3+svn6612/FreeOrion# find . -name \*.cpp | cppcheck --file-list=- -j 4 --enable=portability -q
[universe/ObjectMap.h:53]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:65]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:77]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:80]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:116]: (portability) The extra qualification 'const_iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:122]: (portability) The extra qualification 'const_iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:128]: (portability) The extra qualification 'const_iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:134]: (portability) The extra qualification 'const_iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:140]: (portability) The extra qualification 'const_iterator::' is unnecessary and is considered an error by many compilers.
[universe/ObjectMap.h:143]: (portability) The extra qualification 'const_iterator::' is unnecessary and is considered an error by many compilers.
[Empire/Empire.cpp:2197]: (error) Dangerous iterator comparison using operator< on 'std::deque'.
[GG/src/GIL/extension/io/io_error.hpp:42]: (error) Resource leak: fp
[OIS/src/mac/MacHIDManager.cpp:188]: (error) Uninitialized variable: interface
[UI/SidePanel.cpp:2713]: (error) Instance of 'ScopedTimer' object is destroyed immediately.
[UI/SidePanel.cpp:2740]: (error) Instance of 'ScopedTimer' object is destroyed immediately.
[UI/Sound.cpp:273] -> [UI/Sound.cpp:280]: (error) Iterator 'it' used after element has been erased.
[log4cpp/src/StringUtil.cpp:69]: (error) Memory leak: buffer
[universe/Building.cpp:34]: (error) Instance of 'SetOwner' object is destroyed immediately.
[universe/Building.cpp:138]: (error) Instance of 'SetOwner' object is destroyed immediately.
Nagilum wrote:[universe/ObjectMap.h:53]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
Nagilum wrote:[universe/ObjectMap.h:53]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
It's calling operator++() of the iterator class, not of the map. How is the qualification unnecessary?
Yeah, looks like a false positive to me too. I talked to someone with actual C++ experience and he said it looks ugly/like bad practice but ok. Unfortunately he didn't elaborate yet seemed to agree that it's a false positive.
Thanks to your recent changes and ignoring the iterator calls we're down to
freeorion_0.4.3+svn6620# find . -name \*.cpp | cppcheck --file-list=- -j 4 -q
[FreeOrion/Empire/Empire.cpp:2189]: (error) Dangerous iterator comparison using operator< on 'std::deque'.
[FreeOrion/GG/src/GIL/extension/io/io_error.hpp:42]: (error) Resource leak: fp
[FreeOrion/OIS/src/mac/MacHIDManager.cpp:188]: (error) Uninitialized variable: interface
[FreeOrion/OIS/src/mac/MacHIDManager.cpp:195]: (error) Uninitialized variable: interface
[FreeOrion/UI/Sound.cpp:265] -> [FreeOrion/UI/Sound.cpp:273]: (error) Iterator 'it' used after element has been erased.
[FreeOrion/log4cpp/src/StringUtil.cpp:69]: (error) Memory leak: buffer
Of course these are just hints which I hope are somewhat useful. I was thinking of providing these from time to time whenever the output changes but if there is no value and just noise I'll stop.
Running cppcheck against svn6631 I see something new (in bold):
[Empire/Empire.cpp:2189]: (error) Dangerous iterator comparison using operator< on 'std::deque'.
[GG/src/GIL/extension/io/io_error.hpp:42]: (error) Resource leak: fp
[UI/Sound.cpp:265] -> [UI/Sound.cpp:273]: (error) Iterator 'it' used after element has been erased. [client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: ''.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: 'OGRE_STATIC_LIB'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_GG_Enum_h_'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_GG_Exception_h_'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_MSC_VER'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_MSC_VER;int64_t'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '__APPLE__;__MACH__'.
That block of code would probably break if you defined a bunch of macros that interfere with the OS selection, but I don't think it's really a problem...
#ifndef FREEORION_WIN32
int main(int argc, char* argv[]) {
// copy command line arguments to vector
std::vector<std::string> args;
for (int i = 0; i < argc; ++i)
args.push_back(argv[i]);
// set options from command line or config.xml, or generate config.xml
if (mainConfigOptionsSetup(args) != 0) {
std::cerr << "main() failed config." << std::endl;
return 1;
}
#else
int wmain(int argc, wchar_t* argv[], wchar_t* envp[]) {
// copy UTF-16 command line arguments to UTF-8 vector
std::vector<std::string> args;
for (int i = 0; i < argc; ++i) {
std::wstring argi16(argv[i]);
std::string argi8;
utf8::utf16to8(argi16.begin(), argi16.end(), std::back_inserter(argi8));
args.push_back(argi8);
}
// set options from command line or config.xml, or generate config.xml
if (mainConfigOptionsSetup(args) != 0) {
std::cerr << "main() failed config." << std::endl;
return 1;
}
#endif
#ifndef FREEORION_MACOSX
// did the player request help output?
if (GetOptionsDB().Get<bool>("help")) {
GetOptionsDB().GetUsage(std::cerr);
return 0; // quit without actually starting game
}
// set up rendering and run game
if (mainSetupAndRunOgre() != 0) {
std::cerr << "main() failed to setup or run ogre." << std::endl;
return 1;
}
return 0;
#endif
}
The problem I see with the original code is on anything other than WIN32, MACOSX or LINUX. I know FO doesn't run on anything else currently but why not make porting easier and the code analyser happy at the same time.
Attachments
[The extension patch has been deactivated and can no longer be displayed.]
I think the existing code is specifically set up to not have a main function on OSX, as it's provided elsewhere, but the source file is still compiled to get the other functions defined in it.
Geoff the Medio wrote:I think the existing code is specifically set up to not have a main function on OSX, as it's provided elsewhere, but the source file is still compiled to get the other functions defined in it.
50 #ifndef FREEORION_MACOSX
51 #ifdef FREEORION_WIN32
52 int wmain(int argc, wchar_t* argv[], wchar_t* envp[]) {
53 // copy UTF-16 command line arguments to UTF-8 vector
54 std::vector<std::string> args;
55 for (int i = 0; i < argc; ++i) {
56 std::wstring argi16(argv[i]);
57 std::string argi8;
58 utf8::utf16to8(argi16.begin(), argi16.end(), std::back_inserter(argi8));
59 args.push_back(argi8);
60 }
61
62 // set options from command line or config.xml, or generate config.xml
63 if (mainConfigOptionsSetup(args) != 0) {
64 std::cerr << "main() failed config." << std::endl;
65 return 1;
66 }
67 #else /* ~FREEORION_WIN32 */
68 int main(int argc, char* argv[]) {
69 // copy command line arguments to vector
70 std::vector<std::string> args;
71 for (int i = 0; i < argc; ++i)
72 args.push_back(argv[i]);
73
74 // set options from command line or config.xml, or generate config.xml
75 if (mainConfigOptionsSetup(args) != 0) {
76 std::cerr << "main() failed config." << std::endl;
77 return 1;
78 }
79 #endif /* ~FREEORION_WIN32 */
80 // did the player request help output?
81 if (GetOptionsDB().Get<bool>("help")) {
82 GetOptionsDB().GetUsage(std::cerr);
83 return 0; // quit without actually starting game
84 }
85
86 // set up rendering and run game
87 if (mainSetupAndRunOgre() != 0) {
88 std::cerr << "main() failed to setup or run ogre." << std::endl;
89 return 1;
90 }
91 return 0;
92 }
93 #endif /* !FREEORION_MACOSX */
The use of the "~" as a secondary negation indicator I just came up with as it also used as negation in certain places and I wanted it to be different from #ifndef - YMMV.
Apart from that I think this might one of the few occasions where stacked #ifdef's are actually clearer/easier to understand than the flat ones. But maybe it's just me seeing that file for the first time.
Attachments
[The extension patch has been deactivated and can no longer be displayed.]
-svn 6773
+svn 6790
[Empire/Empire.cpp:2189]: (error) Dangerous iterator comparison using operator< on 'std::deque'.
[GG/src/GIL/extension/io/io_error.hpp:42]: (error) Resource leak: fp
-[UI/Sound.cpp:265] -> [UI/Sound.cpp:273]: (error) Iterator 'it' used after element has been erased.
+[UI/Sound.cpp:104]: (error) Mismatching allocation and deallocation: array
+[UI/Sound.cpp:107]: (error) Mismatching allocation and deallocation: array
+[UI/Sound.cpp:110]: (error) Mismatching allocation and deallocation: array
+[UI/Sound.cpp:363] -> [UI/Sound.cpp:371]: (error) Iterator 'it' used after element has been erased.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: ''.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: 'OGRE_STATIC_LIB'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_GG_Enum_h_'.
Nagilum wrote:
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: ''.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: 'OGRE_STATIC_LIB'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_GG_Enum_h_'.
[/code]
Nagilum wrote:
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: ''.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: 'OGRE_STATIC_LIB'.
[client/human/chmain.cpp:93]: (error) Invalid number of character ({) when these macros are defined: '_GG_Enum_h_'.
[/code]
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: ''.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: 'FREEORION_MACOSX'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: 'FREEORION_MACOSX;check'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: 'OGRE_STATIC_LIB'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: '_GG_Enum_h_'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: '_GG_Exception_h_'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: '_MSC_VER'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: '_MSC_VER;int64_t'.
[client/human/chmain.cpp:94]: (error) Invalid number of character ({) when these macros are defined: '__APPLE__;__MACH__'.