Skip to content

Fix: Clang warnings related to signed-unsigned conversions #779

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.

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

Conversation

pniedzielski
Copy link
Collaborator

This PR fixes warnings that Clang emits due to signed-unsigned conversions.

Please see commit messages for more details.

This patch adds two explicit casts to the `put` function of both
`FileBackedStorage` and `InMemoryStorage`, silencing warnings from
Clang, and making it explicit that we have a type boundary between one
interface that provides us with a signed `int`, and another that
provides us with an `unsigned int`.  A cursory examination of both of
these interfaces does not give a clear answer to whether we can (or
should) change their type; instead we make explicit the unsafe
type-conversion behavior here.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
This patch fixes the last of the signed-unsigned comparison warnings in
our unit tests.  We mostly do this by changing the types that are being
compared, rather than introducing new casts.  However, when we need to
introduce a new cast, we choose to cast the reference value we are
comparing to, rather than the return value from the
component-under-test.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
@pniedzielski pniedzielski requested a review from a team as a code owner June 13, 2025 19:34
@pniedzielski pniedzielski force-pushed the published/fix/warnings/signed-unsigned branch from 9a4182e to e392bf0 Compare June 13, 2025 20:17
Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 2763 of commit e392bf0 has completed with FAILURE

Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

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

Have a few ideas

This patch removes two asserts from the `bmqa_message` unit test that
are now trivially true due to the previous warning fixes.  One is a
nonnegative check that is now trivially true by virtue of the type now
being unsigned; one is a postcondition check that is necessarily true
now.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
@pniedzielski pniedzielski force-pushed the published/fix/warnings/signed-unsigned branch from e392bf0 to d5e6b4a Compare June 17, 2025 18:18
Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 2773 of commit d5e6b4a has completed with FAILURE

@pniedzielski pniedzielski merged commit 3a74a48 into bloomberg:main Jul 7, 2025
42 of 43 checks passed
@pniedzielski pniedzielski deleted the published/fix/warnings/signed-unsigned branch July 7, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants