After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752814 - Fix build of gtk/gtkcsstypesprivate.h
Fix build of gtk/gtkcsstypesprivate.h
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Themes
3.17.x
Other Windows
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-07-24 08:55 UTC by Fan, Chun-wei
Modified: 2015-08-05 21:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix gtk/gtkcsstypesprivate.h on C89 (5.43 KB, patch)
2015-07-24 09:04 UTC, Fan, Chun-wei
none Details | Review
gtk/gtkcsstypesprivate.h: Use #define's instead of enums to avoid depending on compiler-dependent behavior (6.64 KB, patch)
2015-08-05 16:16 UTC, Fan, Chun-wei
committed Details | Review

Description Fan, Chun-wei 2015-07-24 08:55:10 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!
Comment 1 Fan, Chun-wei 2015-07-24 09:04:11 UTC
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!
Comment 2 Emmanuele Bassi (:ebassi) 2015-07-24 10:17:34 UTC
(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.
Comment 3 Emmanuele Bassi (:ebassi) 2015-07-24 10:19:00 UTC
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?
Comment 4 Benjamin Otte (Company) 2015-07-27 23:07:08 UTC
(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.
Comment 5 Matthias Clasen 2015-08-05 11:43:11 UTC
Fan, are you going to do a patch for this along the lines of what comment 4 says ?
Comment 6 Fan, Chun-wei 2015-08-05 16:16:22 UTC
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!
Comment 7 Matthias Clasen 2015-08-05 21:32:36 UTC
Review of attachment 308803 [details] [review]:

Looks fine, thanks.
Comment 8 Fan, Chun-wei 2015-08-05 21:51:14 UTC
Hi Matthias,

Thanks for the review, and sorry again for the delay.

The patch was pushed as f0a1a0c.

With blessings, thank you!