GNOME Bugzilla – Bug 752814
Fix build of gtk/gtkcsstypesprivate.h
Last modified: 2015-08-05 21:51:14 UTC
Hi, The changes introduced by commits 6323010 and 6c862f2 introduced new enums that actually shifted beyond the 32-bit boundary, which caused an int overflow during the build, since on C89/C90 enums are only guaranteed to be 32-bit int's at the most. These (in particular 6323010, which I think caused the complaint initially) caused the line G_STATIC_ASSERT(sizeof (GtkCssChange) >= 8); to error with these on Visual Studio Builds: ... <snip> 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(69) : warning C4341: 'GTK_CSS_CHANGE_TIMESTAMP' : signed value is out of range for enum constant 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(69) : warning C4309: 'initializing' : truncation of constant value 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(70) : warning C4341: 'GTK_CSS_CHANGE_ANIMATIONS' : signed value is out of range for enum constant 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(70) : warning C4309: 'initializing' : truncation of constant value 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(73) : warning C4341: 'GTK_CSS_CHANGE_RESERVED_BIT' : signed value is out of range for enum constant 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(73) : warning C4309: 'initializing' : truncation of constant value 2>c:\gtk.aio\gtk+-3.17.5\gtk\gtkcsstypesprivate.h(76) : error C2118: negative subscript <snip> ... Reading the comments on that line where it errored out says that there is the need to change to defines if the compiler complains. I will post a proposed patch shortly. With blessings, thank you!
Created attachment 308062 [details] [review] Fix gtk/gtkcsstypesprivate.h on C89 Hi, This is my proposed patch that worked for me. I only #ifdef'ed for Visual Studio since that and Linux-GCC were the only compilers that I had direct access to, but I guess this may hit other compilers as well, so it might be so that we have to used #defines for the values. There were 2 items though that I had to change the name a bit (as one might see in the other #ifdef item below), so that we don't get macro redefinintion warnings, and I had GtkCssChange being typedef'ed to a guint64, as that is what I would reckon to be the best type for this case (when we can't just use the enum). Please let me know if this approach is okay, and whether we should just forgo the #ifdef's and just use the #define's. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #0) > The changes introduced by commits 6323010 and 6c862f2 introduced new enums > that actually shifted beyond the 32-bit boundary, which caused an int > overflow during the build, since on C89/C90 enums are only guaranteed to be > 32-bit int's at the most. Not really. ISO C90 says that the enumeration type must be compatible with an int, and that the actual storage is compiler-dependent. C99 introduced better wording, which implies that compilers can extend the storage type to fit the biggest value. Still, enumeration storage size in C is a minefield of compiler-dependent behaviour. I guess this is going to be problematic when building on 32 bit architectures with MSVC anyway.
Review of attachment 308062 [details] [review]: ::: gtk/gtkcsstypesprivate.h @@ +30,3 @@ typedef struct _GtkStyleProviderPrivate GtkStyleProviderPrivate; /* dummy typedef */ +#ifdef _MSC_VER It would be helpful to leave a comment here, explaining why. Alternatively, we could just drop the enumeration type and just use #defines. @@ +50,3 @@ +#define GTK_CSS_CHANGE_PARENT_NAME (1ULL << 15) +#define GTK_CSS_CHANGE_PARENT_REGION GTK_CSS_CHANGE_PARENT_NAME +#define GTK_CSS_CHANGE_STATE (1ULL << 6) Why is this rename needed? @@ +62,3 @@ +#define GTK_CSS_CHANGE_PARENT_SIBLING_NTH_CHILD (1ULL << 26) +#define GTK_CSS_CHANGE_PARENT_SIBLING_NTH_LAST_CHILD (1ULL << 27) +#define GTK_CSS_CHANGE_SIBLING_NTH_LAST_CHILD (1ULL << 12) Same as above: why is this needed? Can't we not rename the enumeration value as well?
(In reply to Emmanuele Bassi (:ebassi) from comment #3) > Alternatively, we could just drop the enumeration type and just use #defines. > That's what I think we should do. It does not make sense to maintain 2 different compiler-specific paths when it's not necessary.
Fan, are you going to do a patch for this along the lines of what comment 4 says ?
Created attachment 308803 [details] [review] gtk/gtkcsstypesprivate.h: Use #define's instead of enums to avoid depending on compiler-dependent behavior Hi, I'm sorry for the delay, was caught up with some things... With blessings, thank you!
Review of attachment 308803 [details] [review]: Looks fine, thanks.
Hi Matthias, Thanks for the review, and sorry again for the delay. The patch was pushed as f0a1a0c. With blessings, thank you!