OpenCyphal / libcanard

A compact implementation of the Cyphal/CAN protocol in C for high-integrity real-time embedded systems
http://opencyphal.org
MIT License
327 stars 192 forks source link

The tests enable -Werror but generate warnings #201

Closed joshvazquez closed 1 year ago

joshvazquez commented 1 year ago

The tests enable -Werror here: https://github.com/OpenCyphal/libcanard/blob/master/tests/CMakeLists.txt#L39

The tests also generate warnings, such as unused variable _ here, conversion to long unsigned int from const int may change the sign of the result here. So the tests fail with warnings as errors.

Is this expected? I am using libcanard in CI/CD and would like to keep the tests enabled.

pavel-kirienko commented 1 year ago

This is not expected. The code should be updated to eliminate the warnings you listed. A pull request would be appreciated.

joshvazquez commented 1 year ago

Can't explain this.

Sign conversion warning:

const auto new_size = size_before + static_cast<std::size_t>(ret); // <-- here
enforce((ret < 0) || (new_size == que_.size),
    "Unexpected size change after push");

OK:

const auto num_added = static_cast<std::size_t>(ret);
enforce((ret < 0) || (size_before + num_added == que_.size),
    "Unexpected size change after push");

Compiler magic?

pavel-kirienko commented 1 year ago

Evidently, integer promotion changes the type of the result, which affects the warning.

joshvazquez commented 1 year ago

I'll submit a PR tomorrow on my work account. @joshvazquez-amzn