GNOME Bugzilla – Bug 757374
macros: clean up "inline" mess
Last modified: 2017-11-21 12:52:27 UTC
GLib redefines 'inline' to __inline or other things that it expects may be appropriate for the given compiler and flags combination, but it does this from gutils.h. That means that this will work for any header that gets included after gutils.h, but not ones that come before it (such as gtypes.h). This is causing build failures due to the newly added use of 'inline' in the overflow-checking integer arithmetic helpers. Move it to gtypes.h so that it gets done earlier.
Created attachment 314501 [details] [review] macros: move our messing about with 'inline'
In fact, I think I will try to come up with a consistent approach for cleaning up all of the weird cases here.
Created attachment 315131 [details] [review] GTrashStack: uninline and deprecate Deprecate GTrashStack and remove the inline implementations for the functions. This will help us clean up the mess that is inline functions in GLib. Because of how G_INLINE_FUNC worked, we have these functions on our ABI, so we must continue to export them as normal functions. We are safe to remove the inline versions, however, because any existing binaries will continue to carry them and any new builds will just start using the non-inline versions.
Created attachment 315132 [details] [review] gutils: clean up bit funcs inlining mess gutils.h and gutils.c define three utility functions as inlines that are also exported via the ABI. This is done via complicated G_INLINE_FUNC and G_IMPLEMENT_INLINES logic. In order to be able to remove this mess, we create a another convoluted but slightly cleaner approach: write straight-up inline versions of the functions named _impl() in the header. Define macros with the "public" function names that call these inlines. From the .c file, export the ABI versions of these functions, implemented using the _impl() version.
Created attachment 315133 [details] [review] GLib: clean up the "inline" mess once and for all It's been a long time since we've been unconditionally saying "static inline" in GLib headers without complaints so it's safe to assume that all compilers that we care about support this. One thing that is not yet totally supported is the unadorned use of the word "inline". Depending on the flags (-std=c89, for example), even GCC will complain about this. Detect missing C99 support and define "inline" to "__inline" in that case. Some research shows "__inline" appears to be the most widely-supported keyword here, but we may need to tweak this if we get some reports of breakage. Clean up all of the configure checks around this and define G_CAN_INLINE unconditionally. Unfortunately, we must assume that some people are still using G_IMPLEMENT_INLINES, we must continue to implement that (including undefining G_CAN_INLINE and redefining G_INLINE_FUNC) if requested. It is not our intent to break existing users of the old-style G_INLINE_FUNC approach and if that has happened, we may need to make some further adjustments.
Review of attachment 315133 [details] [review]: That's a lot of crufty code gone! This looks reasonable to me. Just one question: ::: glib/gmacros.h @@ +55,3 @@ +#define G_CAN_INLINE +#if __STDC_VERSION__ > 199900 +#undef inline Hum...why do we need to do this?
Review of attachment 315131 [details] [review]: GTrashStack seems to date far, far back...and any performance things like this should really have several clear and active users. This one is small enough one could easily just make it a copylib. However, I did find at least one user in a quick search: https://golang.org/test/bench/shootout/k-nucleotide.c
Review of attachment 315132 [details] [review]: OK.
Review of attachment 315133 [details] [review]: I needed to apply all 3 patches (315131 + 315132 + 315133) but after applying them all this morning I can confirm that git master now (almost) builds again with MSVC-8. The one remaining issue is that G_CAN_INLINE has now been #defined in 'glib/gmacros.h' - but for a Windows build, it was already #defined in 'glib/glibconfig.h.win32.in'. The 2 definitions conflict with each other - but if I remove the glibconfig one, everything now builds again (so that'll need to get done upstream) One problem (that only affects me) is that after I apply patches locally, I usually end up with line-ending issues (i.e. conflicting line ending styles in the same file). So (for me) it's usually better if I abandon my recently added patches and pull them in later, once they've been applied upstream. Are we anywhere close to applying them upstream yet?
(In reply to Colin Walters from comment #6) > +#if __STDC_VERSION__ > 199900 > +#undef inline > > Hum...why do we need to do this? Are you asking about the check or the undef? If it's about the check, then it's mentioned in the commit message: if you use -std=c89 then you'll get a warning about using "inline". If it's about the #undef then the answer is "because that's what we were doing before". I could just as well remove it.
(In reply to Allison Ryan Lortie (desrt) from comment #10) > If it's about the #undef then the answer is "because that's what we were > doing before". I could just as well remove it. Yeah, I meant the #undef. That's just a relic from before when we wanted to control the meaning, right? Since it's a compiler keyword by default and not actually a macro it shouldn't break anything, but since it's not needed as far as wel know, let's kill it?
Sure.
Attachment 315131 [details] pushed as 0bfbb0d - GTrashStack: uninline and deprecate Attachment 315132 [details] pushed as 9834f79 - gutils: clean up bit funcs inlining mess Attachment 315133 [details] pushed as db2367e - GLib: clean up the "inline" mess once and for all
I updated from git this morning and I'm happy to confirm that git master is now building again with MSVC - with one proviso... The following line still needs to get removed (line 189 of glib/glibconfig.h.win32.in):- #define G_CAN_INLINE 1 The definition for G_CAN_INLINE has been moved to 'glib/gmacros.h'. So the original definition is no longer needed.
Created attachment 316403 [details] [review] glibconfig.h.win32.in: remove G_CAN_INLINE We now define this unconditionally in gmacros.h. Thanks to John Emmas for the tip.
Comment on attachment 316403 [details] [review] glibconfig.h.win32.in: remove G_CAN_INLINE Attachment 316403 [details] pushed as f2fb877 - glibconfig.h.win32.in: remove G_CAN_INLINE
*** Bug 570072 has been marked as a duplicate of this bug. ***
*** Bug 614326 has been marked as a duplicate of this bug. ***
*** Bug 737863 has been marked as a duplicate of this bug. ***