Some errors found while checking with static code checker (CppCheck)

[AVE]

I just downloaded latest git source and ran check in CodeLite.

1. Missing bounds check for extra iterator increment in loop in crafting.cpp (line 520). Should test for end-of-array there?

2. Duplicate if expressions in defence.cpp (line 449 and line 470). Last if should be removed?

3. Same expression on both sides of && in inventory_ui.cpp (line 669). Should be (iCompareX != -999 && iCompareY != -999)?

4. main_menu.cpp, game::opening_screen() - DIR resource leak. Minor, but still.

5. Same expression on both sides of && in map.cpp (line 2535). A little tricky, macro is automatically expanded by code analyzer. Should be !INBOUNDS(p_it->x, p_it->y) ?

6. Duplicate branches for if and else in mapgen.cpp (line 11657). Not sure what this code does, why checking here anyway?

7. Duplicate if expressions in npcmove.cpp (line 1157). Should be (y > posy) either in line 1157 or line 1159.

8. Boolean result used in bitwise operation in npctalk.cpp (lines 2036, 2030). Should use parenthesis?

9. Consecutive return operator in player.cpp (line 5229) is unnecessary.

Also, a lot of %struct_or_class%::%member% is not initialized in the constructor. Should I list them?

Also, many variables are declared, but never used in code. Should I list them?

[kevin.granade]

1. I expect you’re right, it should test for end and return NULL if it finds it.
   1.a The next method down shuld probably do the same with testing for begin().
2. Yes it looks like the second one should never trigger.
3. Yes
4. Leaks are bad mmmk
5. Yipes, that’s a good catch. This is a danagerous error, possibly even a crash bug.
6. I suspect the first instance should be t_rock, which is a rock wall.
7. Yes, looks like line 1159 should be reversed.
8. I’m not seeing anything that fits the bill, I suspect the line numbers are shifted.
9. Yes that’s definitely redundant.

We definitely need to clean those uninitialized members up, I should probably just run CppCheck and clean those up myself.

Feel free to make a PR removing any and all unused variables, otherwise I’ll run an analyser and nuke them all myself.

Thanks so much for bringing these together, I keep meaning to run the codebase through more analysers, but never seem to have time

[AVE]

8. I’m not seeing anything that fits the bill, I suspect the line numbers are shifted.

Of course they are.
It should be line 2026 instead of 2036. But still, I don’t see a reason why 2028 is ignored when 2030 is flagged as such. Oh, I just understand. Macros are expanded and c_white suddenly became COLOR_PAIR(1) | A_BOLD, and the whole expression on 2026 is now colors[ch] == COLOR_PAIR(1) | A_BOLD && …etc, which is the reason of flagging this. Parentheses should be used in color.h, like that:
c_white = (COLOR_PAIR(1) | A_BOLD)

Feel free to make a PR removing any and all unused variables, otherwise I’ll run an analyser and nuke them all myself.

While I would like to, I prefer to pass this to someone that has better view of how this project develops, because, well, some of that variables can be part of yet-unwritten-code from another contributor.

Thanks so much for bringing these together, I keep meaning to run the codebase through more analysers, but never seem to have time

If I manage to run a PVS-studio (another static code analyzer) in a MSVS2010 Express (which is not a trivial task, because it is a plugin, and plugins are not available in Express MSVS. It has a backdoor-way of running it, but as for me, too overcomplicated), I’ll post anything important that it can find in the latest sources.

[kevin.granade]

No problem, you’ve done most of the work here, I’ll throw up a patch crediting you soon.