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 'Wconversion' warns: static casting doubles #212

Closed
wants to merge 5 commits into from

Conversation

melvinhe
Copy link
Contributor

*Issue number of the reported bug or feature request: #87 *

Describe your changes
This is a small good first issue contribution to fix -Wconversion warnings that pollutes the build log.

This particular PR deals with the following warnings:

/home/runner/work/blazingmq/blazingmq/src/groups/mwc/mwcu/mwcu_printutil.cpp:186:13: warning: conversion from ‘BloombergLP::bsls::Types::Int64’ {aka ‘long long int’} to ‘double’ may change value [-Wconversion]
  186 |             bytes / bsl::pow(1024., static_cast<double>(unit)));
/home/runner/work/blazingmq/blazingmq/src/groups/mwc/mwcu/mwcu_printutil.cpp:275:48: warning: conversion from ‘BloombergLP::bsls::Types::Int64’ {aka ‘long long int’} to ‘double’ may change value [-Wconversion]
  275 |         const bsls::Types::Int64 quot = lround(timeNs /
/home/runner/work/blazingmq/blazingmq/src/groups/mwc/mwcu/mwcu_printutil.cpp:284:57: warning: conversion from ‘BloombergLP::bsls::Types::Int64’ {aka ‘long long int’} to ‘double’ may change value [-Wconversion]
  284 |             (static_cast<double>(timeNs - quot * div) / div) *

Testing performed
N/A

Additional context
Converting from BloombergLP::bsls::Types::Int64 (a 64-bit integer) to double (a 64-bit floating-point) can potentially lead to a loss of precision, but it depends on the size of the integer value used in this context. The division involving another double should already result in similar loss of precision though, so this may be fine. Please check this before merging.

@melvinhe melvinhe requested a review from a team as a code owner March 14, 2024 05:38
@678098 678098 self-requested a review March 14, 2024 12:34
@678098 678098 self-assigned this Mar 14, 2024
678098
678098 previously approved these changes Mar 14, 2024
@pniedzielski pniedzielski self-requested a review March 14, 2024 14:52
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

🎉

@melvinhe, could you run clang-format on these changes? Everything looks good except for a failing format check on our CI.

678098 and others added 3 commits March 14, 2024 11:42
Signed-off-by: Evgeny Malygin <[email protected]>
Signed-off-by: Evgeny Malygin <[email protected]>
Signed-off-by: Evgeny Malygin <[email protected]>
@melvinhe
Copy link
Contributor Author

@pniedzielski Thanks, just ran the format check with clang-format for the CI. See: Fix 'Wconversion' warns: static casting doubles #214

@pniedzielski
Copy link
Collaborator

@melvinhe Thanks! Formatting checks pass now, but I think you had an issue with rebasing, since you've recommitted some changes that were merged into main, which results in merge conflicts when we try to merge this PR. Can you make sure your branch contains only a single commit, squashing your two patches into just one, and not including commits 83c1293, 6f99cd4, and
1f8f881?

If you're not confident with these git operations, I can walk you through it.

@melvinhe
Copy link
Contributor Author

@pniedzielski Sure, after seeing the 3 recent commits added recently, I updated my forked repo main branch to be up to date with bloomberg/blazingmq:main.

Since my old 'wconversion_fix' branch was causing issues when merging, I created a new 'wconv_fix' branch from my local main, manually made my previous casting and formatting edits, and pushed & made a new PR. I realize now that this may be more of a roundabout way of doing things, but the new PR seems to be passing all the Tests other than the integration test that a different commit broke: #214

Should I just deal with the 'wconv_fix' branch than and get rid of my old 'wconversion_fix' branch? Or how else should I remedy this. Thanks!

@pniedzielski
Copy link
Collaborator

@melvinhe We'll work on #214, no worries.

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