2

I got a little confused when I read this macro : #define g_once_init_enter(location) which is defined in glib library.

#define g_once_init_enter(location)                            \
  (G_GNUC_EXTENSION({                                          \
  G_STATIC_ASSERT(sizeof *(location) == sizeof(gpointer));     \
  (void) (0 ? (gpointer) *(location) : 0);                     \
  (!g_atomic_pointer_get (location) &&                         \
  g_once_init_enter (location));                               \
}))

what's the effect of this line :(void) (0 ? (gpointer) *(location) : 0); and the last line is g_once_init_enter(location) again, is it a dead loop? Thanks for your reply.

MFisherKDX
  • 2,840
  • 3
  • 14
  • 25
Kevin.WU
  • 23
  • 4

2 Answers2

3

This is some seriously ugly code. Line by line:

  • G_GNUC_EXTENSION is apparently the same thing as __extension__, which is a dirty trick to hide bad code underneath the carpet. It tells the gcc compiler to treat non-standard code as if it is proper C.
  • G_STATIC_ASSERT(sizeof *(location) == sizeof(gpointer)); is a pre C11 static assert that will expand to some manner of cryptic compiler error in case the pointed-at data is not of the same size as type gpointer. It's some attempt to obtain a tiny bit of type safety.
  • (void) (0 ? (gpointer) *(location) : 0); is some botched attempt to achieve type safety, by writing a line that is never executed, but contains a cast. Notably, almost any type in C can be cast to almost any other type without raising a compiler error, so this line doesn't achieve much at all.
  • The rest are two function calls where g_once_init_enter is only executed in case g_atomic_pointer_get returns 0/NULL.

Overall, the macro tries to achieve type safety in C but doesn't quite manage. Mostly because it is kind of mission impossible, at least pre C11.

Notably, it is questionable to have a type named gpointer, as it means that either the type is not a pointer at all, or it is a pointer hidden behind a typedef. In either case, very bad practice.

In modern C you could likely replace this whole mess with this:

#define g_once_init_enter(location) \
  _Generic(*(location), gpointer :  \
  !g_atomic_pointer_get (location) && g_once_init_enter (location) )
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • Could do without the value judgements. GLib explicitly has to deal with pre-C11 C and a variety of compilers and platforms which don’t support the latest features. The type check in the ternary is effective if various additional compiler warnings are enabled. – Philip Withnall Feb 23 '18 at 14:27
  • @PhilipWithnall I'm sure they had their reasons but that doesn't make the code good, nor good practice. When posting this on the internet, it is important to label bad code "bad", or otherwise some beginner will see it and start writing similar programs. That happens all the time, and then we have to un-teach the beginners, or worse: we have to maintain the code written by them. As for the type check in the conditional operator, I very much doubt it is of much use as they've already told gcc to get lost, by killing `-pedantic`. – Lundin Feb 23 '18 at 14:33
  • Firstly, I very much hope that people are not copying GLib’s code here; they should be #includeing it/linking to it instead (if they need the cross-platform and compiler support). – Philip Withnall Feb 23 '18 at 15:51
  • Secondly, ‘good’ and ‘bad’ are value judgements. Saying some code is not best practice given the availability of a C11 compiler is not a value judgement. Saying that some code has had to jump through hoops to achieve macro type checking given restrictions on compiler and platform support, and that people shouldn’t copy it unless their environment is also subject to those restrictions, is also not a value judgement. I am certain that the type checking in the ternary works, because I tested it the other week (https://bugzilla.gnome.org/show_bug.cgi?id=793399#c2). – Philip Withnall Feb 23 '18 at 15:51
  • @PhilipWithnall Problem 1 is that you have a type named `gpointer`, for which no sound reason can exist and this is not in the slightest subjective. -> – Lundin Feb 23 '18 at 16:16
  • Ignoring that, there's a whole lot of lvalues in C that are compatible with each other given a cast. You can even wildly cast between fixed and floating point. And then there's pointer-to-pointers. And then there's qualified types and pointers. There's aggregates versus scalars. And so on. As I said, type safety is mission impossible. At best, this check manages to tell the difference between arithmetic types and pointers. However, an explicit cast can introduce or hide bugs. – Lundin Feb 23 '18 at 16:17
  • Now what you _can_ do instead of this, short of _Generic, is not to pass arguments by value, but through pointers. Because the C pointer type system is much stricter than the value type system. Also, pre-C11 gcc had various evil non-standard ways to solve the problem such as `typeof`. Since the code is already relying both on gcc and on non-standard extensions, that might be an option. – Lundin Feb 23 '18 at 16:20
  • `gpointer` is equivalent to `void*` and exists for consistency with the other `g`-prefixed types. Nobody said this set of type checks was perfect, but it does catch some simple errors. We can’t change the API of this macro, but we could use `typeof()` if available (a la https://stackoverflow.com/a/8154822/2931197), or `_Generic` if available, to do better checks, indeed. Bug filed upstream: https://bugzilla.gnome.org/show_bug.cgi?id=793897. – Philip Withnall Feb 27 '18 at 19:41
1

This looks like an implicit type check to me.

The macro obviously expects to be passed the address of a gpointer (whatever that is), but macros don't have argument types, so it can't say that.

Instead it asserts that sizeof *(location) is the same as sizeof(gpointer), which verifies that location can be dereferenced and that what it's pointing to has the right size (otherwise that G_STATIC_ASSERT line wouldn't compile).

Then it makes sure that whatever (location) is pointing to can be converted to gpointer, by compiling the cast (gpointer) *(location). This line has no other effect (and the cast is never reached at runtime); it's only there to make the compiler complain if the cast is somehow invalid.

Macros aren't expanded recursively. The last line, g_once_init_enter(location), is left as-is, so there must be an actual g_once_init_enter function somewhere that can be called here.

melpomene
  • 84,125
  • 8
  • 85
  • 148