Skip to content
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

fix: narrowing conversion in util::compare tests #3342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pbrit
Copy link
Contributor

@pbrit pbrit commented Jul 4, 2024

You can see what I had while compiling on linux-aarch64 (Dev Container on MacOS) in the screenshot.

clangd suggests to add static_cast<int>, but I decided against it since char shall not be used with negative values. I'm not 100% certain about that for char and negatives are a bit of a tricky thing in the standard.

How do you feel about that change? @JohanMabille @Hind-M

Screenshot 2024-07-04 at 4 10 29 PM

@Hind-M
Copy link
Member

Hind-M commented Jul 15, 2024

Hmm, I would rather do something like:
CHECK(cmp_equal(signed char{ -1 }, signed char{ -1 }));
and keep the tests.

@Hind-M Hind-M added the release::bug_fixes For PRs fixing bugs label Jul 15, 2024
@Hind-M
Copy link
Member

Hind-M commented Jul 29, 2024

Hmm, I would rather do something like: CHECK(cmp_equal(signed char{ -1 }, signed char{ -1 })); and keep the tests.

@pbrit What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::bug_fixes For PRs fixing bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants