cppcheck svn6612

Questions, problems and discussion about compiling FreeOrion.

Moderator: Oberlus

Message
Author
User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

cppcheck svn6612

#1 Post by Nagilum »

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.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: cppcheck svn6612

#2 Post by Geoff the Medio »

Nagilum wrote:[universe/ObjectMap.h:53]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
Don't understand that...

Code: Select all

std::map<int, boost::shared_ptr<T> >::iterator::operator++();
It's calling operator++() of the iterator class, not of the map. How is the qualification unnecessary?
[log4cpp/src/StringUtil.cpp:69]: (error) Memory leak: buffer
[universe/Building.cpp:34]: (error) Instance of 'SetOwner' object is destroyed immediately.
That should be a call to UniverseObject::SetOwner(int id), not an object construction...

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6612

#3 Post by Nagilum »

Geoff the Medio wrote:
Nagilum wrote:[universe/ObjectMap.h:53]: (portability) The extra qualification 'iterator::' is unnecessary and is considered an error by many compilers.
Don't understand that...

Code: Select all

std::map<int, boost::shared_ptr<T> >::iterator::operator++();
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

Code: Select all

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.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: cppcheck svn6612

#4 Post by Geoff the Medio »

Nagilum wrote: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.
A few of them were useful; the ScopedTimer logging was broken on those lines.

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6612

#5 Post by Nagilum »

Ok, thanks!
As a result I propose this patch:

Code: Select all

--- log4cpp/src/StringUtil.cpp.orig     2014-01-10 23:27:11.971782845 +0100
+++ log4cpp/src/StringUtil.cpp  2014-01-10 21:49:46.875955548 +0100
@@ -66,6 +66,7 @@
             delete [] buffer;
             buffer = new char[size];
         }
+        delete [] buffer;
     }
 
     std::string StringUtil::trim(const std::string& s) {
I know the delete will never be reached so my commit message would be something like:
"- silence cppcheck false positive about buffer[] memory leak"
Attachments

[The extension patch has been deactivated and can no longer be displayed.]


User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: cppcheck svn6612

#6 Post by Geoff the Medio »

Feel free to commit that, I suppose...

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6612

#7 Post by Nagilum »

Done, thanks! :)

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck

#8 Post by Nagilum »

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__'.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: cppcheck svn6612

#9 Post by Geoff the Medio »

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...

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6612

#10 Post by Nagilum »

This attached patch would fix it. The result would be:

Code: Select all

#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.]


User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: cppcheck svn6612

#11 Post by Geoff the Medio »

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.

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6637

#12 Post by Nagilum »

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.
Ah, I see! Thanks!
Ok, then how about this:

Code: Select all

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.]


User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6612

#13 Post by Nagilum »

Code: Select all

-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_'.

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: cppcheck svn6612

#14 Post by Geoff the Medio »

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]
Does this help?

Code: Select all

Index: client/human/chmain.cpp
===================================================================
--- client/human/chmain.cpp	(revision 6792)
+++ client/human/chmain.cpp	(working copy)
@@ -89,9 +89,9 @@
         std::cerr << "main() failed to setup or run ogre." << std::endl;
         return 1;
     }
+#endif
     return 0;
 }
-#endif
 
 
 int mainConfigOptionsSetup(const std::vector<std::string>& args) {

User avatar
Nagilum
Release Manager, Design
Posts: 212
Joined: Thu Dec 31, 2009 3:25 pm
Location: Germany

Re: cppcheck svn6612

#15 Post by Nagilum »

Geoff the Medio wrote:
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]
Does this help?

Code: Select all

Index: client/human/chmain.cpp
===================================================================
--- client/human/chmain.cpp	(revision 6792)
+++ client/human/chmain.cpp	(working copy)
@@ -89,9 +89,9 @@
         std::cerr << "main() failed to setup or run ogre." << std::endl;
         return 1;
     }
+#endif
     return 0;
 }
-#endif
 
 
 int mainConfigOptionsSetup(const std::vector<std::string>& args) {
I'm afraid no. It just moves the problem down one line.

Code: Select all

[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__'.

Post Reply