[PATCH] Checked sourcecode with cppcheck

[Mr. X]

I checked the sourcecode of 0ad with cppcheck (current git head) once again (#606). I'll attach the log.

The majority of the message are complaints about uninitialized variables in constructor, variable scopes that can be reduced, about C-style pointer castings and functions that can be made const or static. However, among the "conclusive Messages", there are also 29 performance messages, three errors and lots of warnings. I did not verify that all these messages are correct, so there might be a few false positives.

I attached three log files:

• 0ad_full.txt contains all messages from cppcheck.
• 0ad_conclusive.txt contains all messages flagged as "conclusive"
• 0ad_cleaned.txt contains all conclusive message without the three most often shown messages, to make it easier for you to spot the other ones

[ben]

In 13337:

Fixes memory leak and removes unused code in WaterManager, fixes #1891, refs #1852

[leper]

In 13413:

Change some size() > 0 to Based on patches by kuranes and Markus. Refs #1852, #1923.

[ben]

In 13418:

Use prefix increment operator for iterators (for consistency), refs #1852, #1064

[ben]

In 13419:

Makes some classes NONCOPYABLE, based on patch by Markus, refs #1852

[ben]

In 13420:

Fixes some unconventional assignment operators, patch by Markus, refs #1852.
Fixes typo in test_ShaderManager

[ben]

In 13421:

Catch exceptions by reference (by convention), based on patch from Markus, refs #1852

[historic_bruno]

Thanks for the patches Markus, I think that leaves only memleak-sectionlayout.patch​ and remove-unused.patch​​:

• For the unused variables, we have to be very careful. Sometimes they are hiding logic errors, things were not implemented fully or correctly, or intentionally left as reminders.
• The SectionLayout patch causes a crash when closing Atlas.

[Markus]

The file memleak-sectionlayout.patch can be ignored. Reading more about wxWidgets it seems to somehow free registered child widgets automatically when the parent window is destroyed. So there is practically no memleak.

Regarding remove-unused.patch:
Sometimes there are comments for something not being implemented etc. I then commented it out. The others are removed as leper said it would being pointless to add more commented out code (especially with revision control).
I think unused variables should be removed if not needed, because it normally is a sign for an logical error. (And as such a warning is issued by the compiler.)

[historic_bruno]

Replying to Markus:

Regarding remove-unused.patch:
Sometimes there are comments for something not being implemented etc. I then commented it out. The others are removed as leper said it would being pointless to add more commented out code (especially with revision control).
I think unused variables should be removed if not needed, because it normally is a sign for an logical error. (And as such a warning is issued by the compiler.)

This is true, but who is going to read the revision log to find random chunks of code that were removed? Or scan the source code looking for commented out variable definitions? At least with compiler warnings there is a chance they will annoy someone enough to properly understand and fix them :)

I'd rather have Yves weigh in on the JS debugger related code, wraitii for the water manager related code. GUItext.cpp and ComponentManagerSerialization.cpp I'm not sure about, but those in CCmpAIManager.cpp and MessagePasserImpl.cpp can definitely go.

[ben]

In 13663:

Removes more unused variables, based on patch by Markus, refs #1852.
Adds TODOs to use POT textures in fancy water rendering.

[historic_bruno]

Note: the unused code in TerrainRenderer.cpp was obviously an unfinished attempt to use POT textures for fancy water rendering, so I added TODOs for both cases where it's using renderer height and width. That should be fixed anyway for efficiency, but it requires a bit of fiddling with the shaders.

All that's left from the patches is the unused vars in the JS debugger, I'd like to get Yves' input on that, whether it's a bug, unfinished feature, or small oversight.

[Jorma Rebane]

Are there any more patches to be reviewed, or have all of these been committed? So far it looks like all changes have been applied and this patch can be closed?

[ben]

In 14211:

Removes unused var from JS debugger, from patch by Markus. Fixes #1852

[historic_bruno]

Thanks for the patches.

(Please create a new ticket for any future cppcheck results and associated patches, even if it's tomorrow :))