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: data type consistency and Wconv catch #238

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

melvinhe
Copy link
Contributor

@melvinhe melvinhe commented Apr 4, 2024

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

Describe your changes
This PR deals with some ‘size_t‘ and ‘int‘ data type inconsistencies. Between lines 400-437, I fixed the data types to ‘int‘ from ‘size_t‘ to keep consistent with the previous loops in the file.

This PR also catches with the following -Wconversion warnings that pollutes the build logs:

/home/runner/work/blazingmq/blazingmq/src/groups/mwc/mwcc/mwcc_array.t.cpp:633:32: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
  633 |         obj.push_back(TestType(i, s_allocator_p));
      |                                ^
/home/runner/work/blazingmq/blazingmq/src/groups/mwc/mwcc/mwcc_array.t.cpp:638:38: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
  638 |         srcVec.push_back(TestType(10 * i, s_allocator_p));
      |                                   ~~~^~~

Testing performed
N/A

Additional context
An alternative naive fix to the logs would have been casting the first parameters in TestType as an ‘int‘ instead of making the changing the for loops to be of data type ‘int‘ instead of ‘size_t‘. However, k_STATIC_LEN is defined to be ‘const int ::k_STATIC_LEN = 10‘, and most of the for loops in the context use ‘int‘ if they aren't explicitly bounded by or referring to an obj size.

Note that lines 400-437 look hardcoded. Please comment to let me know if I should use or define a local ‘k_NB_ITEMS‘ or some other sort of constant(s) instead of having 50, 100, 150, 200, etc, and I can add this to the PR if needed.

@melvinhe melvinhe requested a review from a team as a code owner April 4, 2024 08:10
@678098
Copy link
Collaborator

678098 commented Apr 4, 2024

Hi @melvinhe! Thank you for contribution!
Regarding this:

Please comment to let me know if I should use or define a local ‘k_NB_ITEMS‘ or some other sort of constant(s) instead of having 50, 100, 150, 200, etc, and I can add this to the PR if needed.

In theory, having constants instead of "magic numbers" is a good thing, but in this particular case tests' code is very-very simple, and introducing constants in the code could make it less readable.

@678098 678098 merged commit d324416 into bloomberg:main Apr 4, 2024
10 checks passed
@pniedzielski
Copy link
Collaborator

Thanks for these PRs @melvinhe !

alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
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