Skip to content

[ZH] Fix loop index comparison warnings #804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[ZH] Fix loop index comparison warnings #804

wants to merge 2 commits into from

Conversation

Mauller
Copy link

@Mauller Mauller commented May 3, 2025 •
edited
Loading

This PR fixes loop index comparison warnings by matching the data type of the loop index to match the comparison values data type.

TODO:

  • Replicate in Generals.

@Mauller Mauller force-pushed the fix-loop-index-warnings branch from 14f7171 to 8aa4016 Compare May 3, 2025 16:32
@Mauller Mauller marked this pull request as ready for review May 3, 2025 16:39
@Mauller Mauller self-assigned this May 3, 2025
@Mauller Mauller added Minor Severity: Minor < Major < Critical < Blocker Build Anything related to building, compiling ZeroHour Relates to Zero Hour labels May 3, 2025
@aliendroid1
Copy link

If you are going to change int to an unsigned type then I think std::size_t would be more appropriate and in line with standard conventions. Where range loops can be used then they should be preferred

xezon
xezon approved these changes May 3, 2025
View reviewed changes
Copy link

@xezon xezon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fairly safe, but I suggest to give it a test for mismatch, especially with a focus on USA Countermeasures.

size_t is identical to std::size_t.

GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp
@@ -413,7 +413,7 @@ static void findHighFileNumber( AsciiString filename, void *userData )

// strip off the extension at the end of the filename
AsciiString nameOnly = filename;
for( Int count = 0; count < strlen( SAVE_GAME_EXTENSION ); count++ )
for( size_t count = 0; count < strlen( SAVE_GAME_EXTENSION ); count++ )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this will call strlen on every iteration...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i think the VC6 version may do that but modern MSVC would likely compile it out as a constant.

GeneralsMD/Code/GameEngine/Source/GameClient/MapUtil.cpp
@@ -579,7 +579,7 @@ Bool MapCache::loadUserMaps()
{
AsciiString endingStr;
AsciiString fname = s+1;
for (Int i=0; i<strlen(mapExtension); ++i)
for (size_t i=0; i<strlen(mapExtension); ++i)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another strlen in condition...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... i didn't want to change too much.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/CountermeasuresBehavior.cpp
@@ -301,7 +301,7 @@ void CountermeasuresBehavior::launchVolley()
Object *obj = getObject();

Real volleySize = (Real)data->m_volleySize;
for( int i = 0; i < data->m_volleySize; i++ )
for( UnsignedInt i = 0; i < data->m_volleySize; i++ )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this will produce the same float value below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory it should as the raw binary values of positive signed int and unsigned int values are the same up to 2.7 billion at least.

@Mauller Mauller force-pushed the fix-loop-index-warnings branch from 8aa4016 to f4c5b85 Compare May 4, 2025 07:26
@Mauller
Copy link
Author

Mauller commented May 4, 2025

Updated this with the inclusion of a cast that was missing in one of the loops.

@Mauller Mauller force-pushed the fix-loop-index-warnings branch from f4c5b85 to c0eff59 Compare May 4, 2025 16:40
@Mauller
Copy link
Author

Mauller commented May 4, 2025

Just rebased off recent main after the other warning fixes were merged.

@Mauller
Copy link
Author

Mauller commented May 4, 2025

Just ran Golden Replay 1 against this with the VC6 build and it did not mismatch.
Not sure if countermeasures were explicitly used by any of the USA players. But they were using every vehicles in the game during the test.

So I think the changes are fairly safe, i can replicate the changes to generals if people are happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aliendroid1 aliendroid1 aliendroid1 left review comments

@xezon xezon xezon approved these changes

Assignees

@Mauller Mauller

Labels
Build Anything related to building, compiling Minor Severity: Minor < Major < Critical < Blocker ZeroHour Relates to Zero Hour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mauller @aliendroid1 @xezon