troydhanson / uthash

C macros for hash tables and more
Other
4.18k stars 926 forks source link

Fix warnings issued by -Wpadded (gcc/clang). #151

Closed silvioprog closed 6 years ago

silvioprog commented 6 years ago

This PR fixes the following errors (passing -Wall -Werror -Wextra -Wpedantic -Wpadded):

src/uthash.h:1177:27: error: padding struct to align ‘tail’ [-Werror=padded]
    struct UT_hash_handle *tail; /* tail hh in app order, for fast append    */
                           ^
src/uthash.h:1204:1: error: padding struct size to alignment boundary [-Werror=padded]
 } UT_hash_table;
Quuxplusone commented 6 years ago

This is a duplicate of #118, where the state is "we'll take a patch, but it's got to preserve ABI compatibility somehow."

silvioprog commented 6 years ago

Hm... I'm not sure about the progress in #118. :confused: However, if understood well, a possible way to fix it would be adding a -DUTHASH_OLD_LAYOUT. If you prefer I can edit my PR implementing the flag UTHASH_OLD_LAYOUT.

silvioprog commented 6 years ago

Hey dude, what do you think about to add -DUTHASH_OLD_LAYOUT as you suggested at #118? It will fix the problem in a simple way, if someone finds a better solution just send a new patch.

Quuxplusone commented 6 years ago

I think this patch would be improved by your adding -DUTHASH_OLD_LAYOUT (under the name -DUTHASH_V2_ABI, perhaps?). I might still be wary of accepting it; but I wouldn't accept it without such a macro. I'd think that we'd want to add that macro to opt-into the old ABI, and then immediately tag a "v3.0.0" release. The other thing to look at here is: Will you just have to do the whole process over again as soon as they invent 128-bit pointers? 128-bit long long? Or can you arrange the elements in a future-proof way that won't warn in the future either? And, what are the performance implications if any? I don't have any benchmarks for uthash. And, check the open issues to see if you can fix all the ABI-related issues in one patch.

silvioprog commented 6 years ago

The commit 93d1dfce03d3ef0796b3e35f250b5536120a88a4 adds the UTHASH_V2_ABI. I'm not sure if it is the best way to fix this problem to finally make uthash conformable to -Wpadded, but it compiles properly and reduced the UT_hash_table size from 64 to 56. Any change in my PR are welcome. :-)

silvioprog commented 6 years ago

Hello @Quuxplusone . It seems the -Wpadded options is a little problematic: https://gnunet.org/bugs/view.php?id=5302 . I'm going to remove it from my building system. :sweat_smile:

Quuxplusone commented 6 years ago

Christian Grothoff is blunter than I am, but I think he's got essentially the right attitude on this one. 😛

silvioprog commented 6 years ago

Hehehe