Fix -Wsign-compare failures on 32 bit architecture

[aditishri18]

Problem:

-Wsign-compare check was enabled to warn when a comparison between signed and unsigned values produce an incorrect result. This PR caused failures on 32bit architecture. To fix the problem, the check was disabled and added as a debug build here.

Solution:

To ensure that the errors listed in PR are fixed, a 32bit build in the CI is required for testing. This will ensure any such code breakage on multiple architectures.

[jhgit]

Removing -Wsign-compare may paper over legitimate problems. The [now closed] PR actually fixes the code that was making invalid assumptions regarding max values (and so allows the code to compile without warnings even when -Wsign-compare is set).

[lrstewart]

We're aware that sign-compare issues may be legitimate. Unfortunately, none of our CI tests currently run on 32-bit architecture, so we can't definitively validate any fixes and can't guarantee -Wsign-compare won't break again in the near future. To address that problem, we're working on adding 32-bit environments to our CI builds. Once that's done, we can re-enable the -Wsign-compare check by default.

[jhgit]

Ok. In the mean time, I re-submitted my changes which should make the code better on 32-bit architectures - PR 3899

I don't understand why the previous PR was closed just because the -Wsign-compare warning was removed. Maybe the thought was that 32-bit fixes won't be considered until a CI for 32-bit is set up?

[lrstewart]

Maybe the thought was that 32-bit fixes won't be considered until a CI for 32-bit is set up?

That's the idea. We can't validate that your changes fix the problem today, and we can't guarantee that we won't accidentally undo them again tomorrow. We need testing.

[jhgit]

That's the idea. We can't validate that your changes fix the problem today, and we can't guarantee that we won't accidentally undo them again tomorrow. We need testing.

Understood. Should I close #3899 or let it sit in the queue for a bit? I won't likely notice right away when/if the project gets the 32-bit CI build in place, so if this gets closed, I won't likely submit changes for 32-bit support until I do notice. Maybe that's just the best way to handle it from the project point of view. If it's going to take a while, maybe it's best to close it to avoid clutter of an aging pull request.

[maddeleine]

Understood. Should I close #3899 or let it sit in the queue for a bit? I won't likely notice right away when/if the project gets the 32-bit CI build in place, so if this gets closed, I won't likely submit changes for 32-bit support until I do notice. Maybe that's just the best way to handle it from the project point of view. If it's going to take a while, maybe it's best to close it to avoid clutter of an aging pull request.

You can close #3899. When we add a 32-bit CI build we will turn on the sign-compare check and either reopen your PR or author it ourselves.