"would not cause it to be binary incompatible with the solaris-studio-built glib" is what I meant

Created attachment 288049 [details] [review]
Add the special case for GCC in the inline function definition as well.

Could anyone review the patch?

I'm surprised to hear that there still exists a compiler that can't handle 'static inline' in headers.

Is this the case for Sun Studio or are we just getting the check wrong?

I'd really prefer to burn down this entire mess and just unconditionally assume that 'static inline' works on all compilers that we care about these days.

fwiw, even MSVC supports this but it seems that (at least at some point in 2008) you needed to write '__inline' instead of 'inline'.

Meanwhile, some files in glib (like gvariant-serialiser.c) make liberal usage of 'static inline' absolutely unconditionally and we've never heard any complaints about that.

I think it's reasonably safe to assume that we can get rid of this mess by now...

The solaris studio compiler will generate a static symbol for a function in a header even when that function is not called by anything. Which is what G_CAN_INLINE detects, and the check works correctly.

The problem is not with the check, but rather with how it is used in the code. Since the declaration tests for G_CAN_INLINE or __GNUC__, but the definition checks only for G_CAN_INLINE.

This patch only makes the definition consistent with the declaration.

Actually, just noticed the patch I sent doesn't actually work on Linux, let me review it again.

Ok, so the problem with my original fix was that it turned on all the non-static function definitions in all the places, causing the symbols to be multiply defined.

That pointed me to a much simpler approach, which is to simply add 

 #  define G_INLINE_FUNC _GLIB_EXTERN
 #  undef  G_CAN_INLINE
 #elif defined (__GNUC__) 
+#  define G_CAN_INLINE
 #  define G_INLINE_FUNC static __inline __attribute__ ((unused))
 #elif defined (G_CAN_INLINE) 
 #  define G_INLINE_FUNC static inline
 #else /* can't inline */

This way, when using gcc where glib was originally compiled with a compiler that can't do G_CAN_INLINE, automatically turns on G_CAN_INLINE whenever it sees gcc, regardless of what config.h says.

(In reply to comment #6)
> The solaris studio compiler will generate a static symbol for a function in a
> header even when that function is not called by anything. Which is what
> G_CAN_INLINE detects, and the check works correctly.

This is really a bug with the compiler, then.  Why would it emit code for a function that nothing will ever call, particularly when that function is marked 'inline'?

To be fair, the C standard doesn't say that it shouldn't. So I'm not sure it can be claimed to be a compiler bug.

Portability-wise, however, I'd argue that coping with non-gcc-compatible behaviors in glib is a worthwhile goal.

Note that glib already copes with the solaris studio compiler, it works fine. The problem I reported here only manifests itself when you use gcc to build code against a solaris-studio-built glib.

To make it more clear:

step 1: build glib with solaris studio compiler and install
step 2: build some other software with gcc and use that glib

because glib was originally configured with solaris studio, it will not have G_CAN_INLINE set, however, when building gcc it will fall on the "elif defined(__GNUC__)" case, which will trigger the declaration as inline but not the definition.

The last change I proposed (#define G_CAN_INLINE unconditionally when __GNUC__) solves that problem by telling glib to use inline when using gcc regardless of what it found at configuration time.

The problem from my standpoint is that we've been discussing another feature that would result in a very large number of static inlines being defined in header files (a macro to avoid the large amount of .h boilerplate required when defining new GObject subclasses).  I'm not sure that I want to hold back on that just because the Sun compiler won't properly support it...

That's the reason that I'd rather clean up this mess once and for all and just start unconditionally depending on the existence of a working (according to our definition of 'working') 'static inline'.

It would be possible to make the declaration/definition of the static inline functions more straight-forward without losing the same level of portability.

here's an idea:

1) drop the "extern" from the macro when it's not supported and just leave it empty. It has the same result and doesn't require special case for the declaration vs the definition.

2) Move the inlines to a separate foo_inline.h file.

3) Conditionally include foo_inline.h from foo.h if inline is supported

4) Unconditionally include foo_inline.h from foo.c

With this changes, the boiler-plate for supporting the static inlines would be drastically reduced to:

--- on foo.h
#ifndef __G_FOO_H__
#define __G_FOO_H__

#include <glib/gutils.h>

G_INLINE_FUNC int foo_func();

#ifdef G_CAN_INLINE
#include "foo_inline.h"
#endif

#endif
---

--- on foo_inline.h
#ifndef __G_FOO_INLINE_H__
#define __G_FOO_INLINE_H__

G_INLINE_FUNC int foo_func() {
   return 42;
}

#endif
---

--- on foo.c
#include "foo.h"
#include "foo_inline.h"

---

Handling of inlines was rearchitected significantly in bug #757374, as desrt was planning to do. There don’t seem to have been any complaints since then, so I guess it worked.

If there are still problems with inlines with the Sun compiler, please open a new bug report. Thanks.

*** This bug has been marked as a duplicate of bug 757374 ***