Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#1852 closed defect (fixed)

[PATCH] Checked sourcecode with cppcheck

Reported by: Mr. X Owned by: ben
Priority: Nice to Have Milestone: Alpha 15
Component: Core engine Keywords: patch
Cc: Patch:

Description

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

Attachments (9)

0ad_cleaned.txt (14.2 KB ) - added by Mr. X 10 years ago.
0ad_conclusive.txt (57.3 KB ) - added by Mr. X 10 years ago.
0ad_full.txt (152.3 KB ) - added by Mr. X 10 years ago.
catch-exception-by-ref.patch (1.3 KB ) - added by Markus 10 years ago.
mark-noncopyable-to-prevent-memleak.patch (6.2 KB ) - added by Markus 10 years ago.
use-stl-empty-not-size.patch (2.1 KB ) - added by Markus 10 years ago.
wrong-operator-signature.patch (2.6 KB ) - added by Markus 10 years ago.
memleak-sectionlayout.patch (787 bytes ) - added by Markus 10 years ago.
remove-unused.patch (5.1 KB ) - added by Markus 10 years ago.

Download all attachments as: .zip

Change History (27)

by Mr. X, 10 years ago

Attachment: 0ad_cleaned.txt added

by Mr. X, 10 years ago

Attachment: 0ad_conclusive.txt added

by Mr. X, 10 years ago

Attachment: 0ad_full.txt added

comment:1 by Kieran P, 10 years ago

Milestone: BacklogAlpha 13

comment:2 by Kieran P, 10 years ago

Milestone: Alpha 13Backlog
Priority: Should HaveNice to Have

comment:3 by ben, 10 years ago

In 13337:

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

by Markus, 10 years ago

by Markus, 10 years ago

by Markus, 10 years ago

by Markus, 10 years ago

Attachment: memleak-sectionlayout.patch added

by Markus, 10 years ago

Attachment: remove-unused.patch added

comment:4 by Markus, 10 years ago

Keywords: review patch added
Summary: Checked sourcecode with cppcheck[PATCH] Checked sourcecode with cppcheck

comment:5 by leper, 10 years ago

In 13413:

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

comment:6 by ben, 10 years ago

In 13418:

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

comment:7 by ben, 10 years ago

In 13419:

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

comment:8 by ben, 10 years ago

In 13420:

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

comment:9 by ben, 10 years ago

In 13421:

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

comment:10 by historic_bruno, 10 years ago

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.

comment:11 by Markus, 10 years ago

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

in reply to:  11 comment:12 by historic_bruno, 10 years ago

Cc: Yves wraitii added

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.

comment:13 by historic_bruno, 10 years ago

Milestone: BacklogAlpha 15

comment:14 by ben, 9 years ago

In 13663:

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

comment:15 by historic_bruno, 9 years ago

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.

comment:16 by Jorma Rebane, 9 years ago

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?

comment:17 by ben, 9 years ago

Owner: set to ben
Resolution: fixed
Status: newclosed

In 14211:

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

comment:18 by historic_bruno, 9 years ago

Cc: Yves wraitii removed
Keywords: review removed

Thanks for the patches.

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

Note: See TracTickets for help on using tickets.