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

AVRO-3616: C++ Fix compilation warnings #1836

Merged
merged 4 commits into from Aug 24, 2022

Conversation

martin-g
Copy link
Member

Jira

Tests

  • My PR updates existing unit tests

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • No need

Replace <boost/test/included/unit_test_framework.hpp> include with <boost/test/included/unit_test.hpp>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Fix the order of constructor parameters

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Aug 23, 2022
@martin-g martin-g requested a review from thiru-mg August 23, 2022 12:59
Those should prevent from introducing new warnings.

-Wextra reports "unused method arguments" and those could not be fixed
without API break.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@@ -70,7 +70,7 @@ static bool validate(const NodePtr &node, SymbolMap &symbolMap) {

node->lock();
auto leaves = node->leaves();
for (auto i = 0; i < leaves; ++i) {
for (long unsigned int i = 0; i < leaves; ++i) {

Choose a reason for hiding this comment

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

Better use size_t, because Node::leaves returns size_t, and Node::leafAt takes a size_t parameter. Or perhaps decltype(leaves).

Choose a reason for hiding this comment

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

On 64-bit Microsoft Windows, size_t is 64-bit, but long unsigned int is 32-bit and would not be pedantically correct. In practice though, it's rather unlikely that a node would have billions of leaves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved! Thanks!

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@KalleOlaviNiemitalo
Copy link

In Validator, there seems to be some inconsistency with std::vector<size_t> counters_ and int64_t count_, too:

  • Validator::countingSetup converts int64_t to size_t: counters_.push_back(static_cast<size_t>(count_));
  • Validator::countingAdvance converts size_t to int: int count = --counters_.back();
  • Validator::unionAdvance converts size_t to int64_t: if (count_ < static_cast<int64_t>(node->leaves()))
  • Validator::unionAdvance converts int64_t to int and that to size_t: setupOperation(node->leafAt(static_cast<int>(count_)));

Those could be fixed in a separate PR, though.

@martin-g
Copy link
Member Author

In Validator, there seems to be some inconsistency with

How did you find those ?
By looking in the code or by using some tool ?

@KalleOlaviNiemitalo
Copy link

I worried that commit e632e65 changing the types of the local variables to size_t in might cause problems if any subsequent code tries to use negative values in those variables. I looked at the remaining parts of the modified methods and did not find any such problems, but found the int count = --counters_.back() assignment in Validator::countingAdvance. That seemed suspicious to me, so I searched for other uses of counters_ and count_ in the Validator files. I didn't even use an IDE, just a Web browser.

@martin-g
Copy link
Member Author

Thanks, @KalleOlaviNiemitalo !
I will leave this for a separate ticket/PR.

@martin-g martin-g merged commit d336c6c into master Aug 24, 2022
martin-g added a commit that referenced this pull request Aug 24, 2022
* AVRO-3616: [C++]: Fix compilation warnings

Replace <boost/test/included/unit_test_framework.hpp> include with <boost/test/included/unit_test.hpp>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3616: [C++]: Fix compilation warnings

Fix the order of constructor parameters

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3616: Enable -pedantic and -Werror for compiler flags

Those should prevent from introducing new warnings.

-Wextra reports "unused method arguments" and those could not be fixed
without API break.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3616: Use size_t consistently for node->leaves()

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
(cherry picked from commit d336c6c)
@martin-g martin-g deleted the avro-3616-c++-fix-compilation-warnings branch August 24, 2022 07:37
@KalleOlaviNiemitalo
Copy link

I filed those as AVRO-3617.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Pull Requests for C++ binding
Projects
None yet
3 participants