GNOME Bugzilla – Bug 765022
GCC 5 generates a type-limits warning in tpaw-account-settings.c
Last modified: 2016-04-15 14:34:31 UTC
Created attachment 325891 [details] [review] Use MIN () instead of CLAMP () for unsigned int types. Below is the gcc error log. My solution was to use MIN on the unsigned types to keep the upper limit constraint. It is probably pedantic, but maybe some day this code will running on >64bit systems. tpaw-account-settings.c: In function ‘tpaw_account_settings_get_int32’: /usr/include/glib-2.0/glib/gmacros.h:282:63: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] #define CLAMP(x, low, high) (((x) > (high)) ? (high) : (((x) < (low)) ? (low) : (x))) ^ tpaw-account-settings.c:883:11: note: in expansion of macro ‘CLAMP’ ret = CLAMP (g_variant_get_uint32 (v), 0, G_MAXINT32); ^ /usr/include/glib-2.0/glib/gmacros.h:282:63: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] #define CLAMP(x, low, high) (((x) > (high)) ? (high) : (((x) < (low)) ? (low) : (x))) ^ tpaw-account-settings.c:887:11: note: in expansion of macro ‘CLAMP’ ret = CLAMP (g_variant_get_uint64 (v), 0, G_MAXINT32); ^ tpaw-account-settings.c: In function ‘tpaw_account_settings_get_int64’: /usr/include/glib-2.0/glib/gmacros.h:282:63: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] #define CLAMP(x, low, high) (((x) > (high)) ? (high) : (((x) < (low)) ? (low) : (x))) ^ tpaw-account-settings.c:921:11: note: in expansion of macro ‘CLAMP’ ret = CLAMP (g_variant_get_uint64 (v), 0, G_MAXINT64);
Review of attachment 325891 [details] [review]: I agree with switching to use MIN instead of CLAMP. ::: tp-account-widgets/tpaw-account-settings.c @@ +881,3 @@ ret = g_variant_get_int32 (v); else if (g_variant_is_of_type (v, G_VARIANT_TYPE_UINT32)) + ret = MIN (g_variant_get_uint32 (v), G_MAXUINT32); I don't think this is right; tpaw_account_settings_get_int32() should return a value that fits in a 32-bit int, so the max should be G_MAXINT32 like it was before, not G_MAXUINT32. What you have here is basically a no-op, ensuring that a 32-bit unsigned int would fit in a 32-bit unsigned int... of course it does! @@ +885,3 @@ ret = CLAMP (g_variant_get_int64 (v), G_MININT32, G_MAXINT32); else if (g_variant_is_of_type (v, G_VARIANT_TYPE_UINT64)) + ret = MIN (g_variant_get_uint64 (v), G_MAXUINT32); G_MAXINT32 @@ +919,3 @@ ret = g_variant_get_int64 (v); else if (g_variant_is_of_type (v, G_VARIANT_TYPE_UINT64)) + ret = MIN (g_variant_get_uint64 (v), G_MAXUINT64); Similarly, G_MAXINT64. (Hence the MIN is really needed here, and not pedantic after all.)
Created attachment 326058 [details] [review] Fixed version of use MIN instead of CLAMP for coercing unsigned ints into signed ints
Someone #gnome-hackers suggested using UINT, but after looking at the return type, yes, its returning a signed int so the check needs to limit to MAXINT(32|64).
Looks good, thanks!