ludocode / mpack

MPack - A C encoder/decoder for the MessagePack serialization format / msgpack.org[C]
MIT License
533 stars 82 forks source link

Compilation with -Wconversion #46

Closed rikvdh closed 8 years ago

rikvdh commented 8 years ago

Hi Nicholas,

In our project we are looking into -Wconversion. But we found some warnings (and in our case errors with -Werror). I see that you explicitly deactivated -Wno-sign-conversion, which surpresses exactly the warnings I found.

Is there a reason to just ignore these warnings? Perhaps casting makes it more portable between build-environments.

include/mpack/mpack.h: In function ‘mpack_tag_array’:
include/mpack/mpack.h:1006:15: error: conversion to ‘uint32_t’ from ‘int32_t’ may change the sign of the result [-Werror=sign-conversion]
     ret.v.n = count;
               ^
include/mpack/mpack.h: In function ‘mpack_tag_map’:
include/mpack/mpack.h:1015:15: error: conversion to ‘uint32_t’ from ‘int32_t’ may change the sign of the result [-Werror=sign-conversion]
     ret.v.n = count;
               ^
include/mpack/mpack.h: In function ‘mpack_tag_str’:
include/mpack/mpack.h:1024:15: error: conversion to ‘uint32_t’ from ‘int32_t’ may change the sign of the result [-Werror=sign-conversion]
     ret.v.l = length;
               ^
include/mpack/mpack.h: In function ‘mpack_tag_bin’:
include/mpack/mpack.h:1033:15: error: conversion to ‘uint32_t’ from ‘int32_t’ may change the sign of the result [-Werror=sign-conversion]
     ret.v.l = length;
               ^
include/mpack/mpack.h: In function ‘mpack_tag_ext’:
include/mpack/mpack.h:1043:15: error: conversion to ‘uint32_t’ from ‘int32_t’ may change the sign of the result [-Werror=sign-conversion]
     ret.v.l = length;
               ^
cc1: all warnings being treated as errors

Kind regards,

Rik

ludocode commented 8 years ago

I think that one was mostly disabled just to avoid the casts. I expected there to be more errors than there actually are with -Wsign-conversion so it should be worthwhile to turn it on and fix the issues. I should be able to do that in the next couple days.

Do you need it for the v0.8.2 release, or are you using develop? The develop branch is quite far ahead of v0.8.2 but still needs a lot of work before it can be released, and I haven't been able to work on it for a few months unfortunately. I'm looking to pick it up again soon though. If you're using v0.8.2 I can implement it on a side branch and then cherry-pick it over to develop.