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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Jan 5 2019, 17:22
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 5 2019, 17:22
Herald added a subscriber: schancel. · View Herald Transcript
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.

Fabien updated this revision to Diff 6516.Jan 5 2019, 18:43

Remove parenthesis and update the loops to use uint8_t pointers

Fabien updated this revision to Diff 6519.Jan 5 2019, 20:55

Missed one loop

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
Fabien updated this revision to Diff 6522.Jan 6 2019, 10:08

Remove changes to cuckoocache_test

Fabien edited the test plan for this revision. (Show Details)Jan 6 2019, 10:11
deadalnix accepted this revision.Jan 6 2019, 18:23
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
Fabien updated this revision to Diff 6528.Jan 6 2019, 20:52

Remove unnecessary parenthesis

This revision was automatically updated to reflect the committed changes.