Page MenuHomePhabricator

Fix Clang -Wcast-align warnings
ClosedPublic

Authored by Fabien on Jan 5 2019, 17:22.

Details

Summary

Remove some c-style cast that raised warnings on clang due to change in
the alignment requirement. Using reinterpret_cast<>() will
prevent these warnings.

Test Plan

Using clang as a compiler:

make check

Ensure that there is no -Wcast-align warning from the compiler on the files that are updated in this diff

Diff Detail

Repository
rABC Bitcoin ABC
Branch
clang_warnings_cast_align
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4427
Build 6919: Bitcoin ABC Buildbot (legacy)
Build 6918: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Jan 5 2019, 17:46

I'm not sure we won anything here. Sure it silenced the warning, but it doesn't looks like anything was done to actually address the problem, or make sure the compiler has the knowledge that there is no problem. As far as I can tell, the only thing that is achieved here is making the code more opaque to the static analyzer in the compiler, unless I am missing something ?

src/netbase.cpp
94 ↗(On Diff #6512)

It looks like you have useless () here

src/test/cuckoocache_tests.cpp
34 ↗(On Diff #6512)

For this one, if would probably be beneficial to change uint256 to use larger integers internally.

This revision now requires changes to proceed.Jan 5 2019, 17:46
Fabien planned changes to this revision.Jan 5 2019, 18:36

You're not missing anything, this is a workaround to silent the warnings while being iso functional.
The reinterpret_castnot being handled by the analyzer is actually a "feature" to explicitly tell clang that you know what you're doing.

I will update the loops to use uint8_t * instead, but regarding the sockaddr* the API seems designed to use these casts ; this code looks pretty standard.

src/test/cuckoocache_tests.cpp
34 ↗(On Diff #6512)

I think it can be build on 32-bits chunks instead of 8-bit, but I need to check the impacts. This change will better suit another diff anyway.
For the moment I can change these loops to fill the buffer in 8-bit steps, avoiding the reinterpret_cast and matching the current interface.

Remove parenthesis and update the loops to use uint8_t pointers

deadalnix requested changes to this revision.Jan 6 2019, 02:22

It seems like you should split the patch into a part that deal with ip addresses and use reinterpet_cast and a part that updates uint256 innards.

src/test/cuckoocache_tests.cpp
215 ↗(On Diff #6519)

Please don't make the code worse to silence a warning.

This revision now requires changes to proceed.Jan 6 2019, 02:22

Remove changes to cuckoocache_test

deadalnix added inline comments.
src/netaddress.cpp
484 ↗(On Diff #6522)

I think you have extra set of () in there.

This revision is now accepted and ready to land.Jan 6 2019, 18:23

Remove unnecessary parenthesis

This revision was automatically updated to reflect the committed changes.