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 #214

Merged
merged 1 commit into from
Mar 14, 2024

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.
Formatted the updates with clang-format.

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
Copy link
Contributor Author

Updated with clang-format, but I included the recent changes made 1 hour ago to avoid conflicts. One of these commits, [More complex "the the" typos], isn't passing all the CI checks though which may be an issue.

Signed-off-by: Melvin He <melvin_he@brown.edu>
Co-authored-by: Patrick M. Niedzielski <patrick@pniedzielski.net>
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.

Apart from the note below looks good. I'm going to fix that issue by force-pushing the change to your wconv_fix branch before merging it into main. If you want to continue working on this branch, beware of this and reset your local wconv_fix to your remote melvinhe:wconv_fix.

@@ -306,4 +308,4 @@ bsl::ostream& prettyTimeInterval(bsl::ostream& stream,
} // close PrintUtil namespace

} // close package namespace
} // close enterprise namespace
} // close enterprise namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Text files should end with newlines, but more importantly this looks like a stray change that wasn't intentional. It's a good idea generally to double check these things (git show, or use git add -p when staging), and that your patches are all and only what you intend to submit. No big issue

@pniedzielski pniedzielski merged commit 3f7d076 into bloomberg:main Mar 14, 2024
7 checks passed
@pniedzielski
Copy link
Collaborator

Formatting checks pass; thank you @melvinhe for your contribution and for helping our build logs be a little tidier!

@678098
Copy link
Collaborator

678098 commented Mar 15, 2024

Thank you for the contribution @melvinhe!
By the way, personally I prefer creating disposable branch names containing current date, so I don't need to reset a branch. I feel that resetting a branch is a somehow risky operation, even the best of us might not notice and delete unmerged code in the branch via reset.

@melvinhe
Copy link
Contributor Author

@678098 Thanks, I appreciate any feedback! Adding disposable branch names with dates seems like a helpful way for keeping track of things.

Would it be wise to delete a branch ('git branch -d' and 'git push origin --delete') after a PR for it is merged then? Also, when making potential future new contributions, if my current forked main is behind the blazingmq repo, would it be wise to 'sync fork -> update branch' on github & 'git pull origin main' in local IDE, and then create a new disposable branch name containing current date from my local main branch?

Is this something I should do for #216 or is it fine as is? Also, do you know why some commits don't pass 1 or 2 of the integration tests but then do in a future commit despite the future commit not changing anything related to what was done in the earlier commit? Thanks!

@678098
Copy link
Collaborator

678098 commented Mar 19, 2024

Would it be wise to delete a branch ('git branch -d' and 'git push origin --delete') after a PR for it is merged then?

Actually, you can avoid this. Usually, existing branches do not take much disk space or harmful to have. Cleaning up them might seem to make a repo look more clean, but in reality you'll just spend your time and attention on something small. Especially if it's your local clone of a repo. And also it might be not a good idea to delete branches on your local clone, because you might accidentally remove something useful without means to return it back.

if my current forked main is behind the blazingmq repo, would it be wise to 'sync fork -> update branch' on github & 'git pull origin main' in local IDE, and then create a new disposable branch name containing current date from my local main branch?

It's okay to sync your fork with GH web interface.
You can do git pull origin main indeed, however, in certain conditions it makes your life more complicated. For example, if you are on a wrong branch now (not main), or if your main branch is diverged from origin/main, your pull will not be fast-forward, and the PR process will be overcomplicated.
I prefer to do something like this:

git fetch origin
git checkout origin/main
git checkout -b <branch_name>

Alternatively, you can always do fast-forward pulls either by passing a flag git pull --ff-only or by configuring it globally git config --global pull.ff only

Is this something I should do for #216 or is it fine as is?

I will look into that PR soon, thank you!

Also, do you know why some commits don't pass 1 or 2 of the integration tests but then do in a future commit despite the future commit not changing anything related to what was done in the earlier commit?

We've changed a behaviour of a tool used for integration tests, but it should be fine now.

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