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 more Wconversion warnings #125

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Conversation

banipreetr
Copy link
Contributor

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

Describe your changes
Follow up to #122, this PR contribution is to fix more -Wconversion warnings that pollutes the build log.

Testing performed
N/A
Additional context
Add any other context about your contribution here.

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.

One small comment

@@ -47,7 +47,7 @@ void TestHelper::printTestName(bslstl::StringRef value)
}

bsl::cout << "\n" << value << "\n";
int length = value.length();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently long unsigned int is identical to size_t, but for the future it's more safe to use size_t here, as length() returns. The reason is that size_t has an explicit contract in its definition size_t can store the maximum size of a theoretically possible object of any type (including array).

https://en.cppreference.com/w/c/types/size_t

There are size_t used in almost all similar cases in the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, thanks!

@banipreetr banipreetr marked this pull request as ready for review October 9, 2023 19:04
@banipreetr banipreetr requested a review from a team as a code owner October 9, 2023 19:04
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.

LGTM!

@678098 678098 merged commit 63fd3c9 into bloomberg:main Oct 10, 2023
@678098
Copy link
Collaborator

678098 commented Oct 10, 2023

@banipreetr thank you for the contribution!

@kaikulimu kaikulimu mentioned this pull request Oct 10, 2023
1 task
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.

2 participants