(In reply to Colin Walters from comment #0)
> Anyways, are there any objections to me copying the systemd code into glib
> and submitting a patch for G_IN_SET?

Nice, I’m in favour of this. I think the patch to convert GLib would have to be manually tweaked rather than just being a Coccinelle dump, though. I’d like that last (dest == begin || dest == end) change dropped, for example.

The implementation of that macro is definitely iffy, though, especially that:

/* If the build breaks in the line below, you need to extend the case macros */

Shouldn't this be a static inline, instead of a pre-processor macro?

I mean, this:

```
static inline bool
g_in_set (int value, int arity, ...)
{
  bool res = false;
  va_list args;

  va_start (args, arity);
  while (arity-- > 0)
    if (value == va_arg (args, int))
      {
        res = true;
        break;
      }

  va_end (args);
  return res;
}
```

is effectively the same, with an arity argument as a cost for a clearer implementation that adds some type safety.

Right, that would give an int-conversion warning if the user passed a pointer.  Which...is probably a good thing?  Are there use cases for `long` though?

(In reply to Emmanuele Bassi (:ebassi) from comment #3)
> is effectively the same, with an arity argument as a cost for a clearer
> implementation that adds some type safety.

What type safety does this add? If anything it drops type safety by passing everything through runtime varargs (and hence everything is treated as an int rather than being automatically promoted to the right size). At least when we use variadic macros, the type checker runs after macro expansion.

(In reply to Emmanuele Bassi (:ebassi) from comment #2)
> The implementation of that macro is definitely iffy, though, especially that:
> 
> /* If the build breaks in the line below, you need to extend the case macros
> */

I wouldn’t say that in particular is iffy. The whole concept of variadic macros is fairly well established, and this failure mode should only occur if more than 20 elements are listed (or if their sizes sum to more than 20*sizeof(int)?). Seems reasonable to me.

(In reply to Colin Walters from comment #4)
> Right, that would give an int-conversion warning if the user passed a
> pointer.  Which...is probably a good thing?  Are there use cases for `long`
> though?

A fun bit would be to allow using (on compatible compiler) C11 _Generic macros wrapping typed inlined functions:

static inline gboolean g_int_in_set (int, ...) { ... }
static inline gboolean g_long_in_set (long, ...) { ... }

#if CAN_USE_C11_GENERIC
#define g_in_set(value,arity,...) \
  _Generic((value), \
    int: g_int_in_set,
    long: g_long_in_set,
    default: g_int_in_set) (value,arity,__VA_ARGS__)
#else
#define g_in_set(value,arity,...) g_int_in_set(value,arity,__VA_ARGS__)
#endif


But that's probably a bit overkill.

(In reply to Philip Withnall from comment #5)
> (In reply to Emmanuele Bassi (:ebassi) from comment #3)
> > is effectively the same, with an arity argument as a cost for a clearer
> > implementation that adds some type safety.
> 
> What type safety does this add?

On the argument you're passing.

> If anything it drops type safety by passing
> everything through runtime varargs (and hence everything is treated as an
> int rather than being automatically promoted to the right size). At least
> when we use variadic macros, the type checker runs after macro expansion.

That's true, but it's still pretty gross.

We could still keep the static inline function and have the arity argument be expanded by a variadic macro expansion, but…

> (In reply to Emmanuele Bassi (:ebassi) from comment #2)
> > The implementation of that macro is definitely iffy, though, especially that:
> > 
> > /* If the build breaks in the line below, you need to extend the case macros
> > */
> 
> I wouldn’t say that in particular is iffy. The whole concept of variadic
> macros is fairly well established

… even though variadic macros are well established, they are not evenly supported across toolchains.

GLib currently says, of the variadic macros requirement:

"Not a hard requirement. GLib can work with either C99 or GNU style varargs macros."
-- https://wiki.gnome.org/Projects/GLib/CompilerRequirements

We can make it a hard requirement, though. I would gladly make C99 a hard requirement, and drop the support for GNU style varargs, to be fair.

Here's the commit where Lennart introduced the macro; no surprises here, just adding for reference:
https://github.com/systemd/systemd/commit/cabb78068899232c152f4585f19d023e373aa73d

However, this commit is interesting:
https://github.com/systemd/systemd/commit/d5b26d50fca744d1b19eb8c125b047c18c61ad94

I was thinking more about why they didn't use an inline function, and this may be part of the answer:
https://stackoverflow.com/questions/25482031/inlining-of-vararg-functions

And quickly testing that code with gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1), indeed the vararg function isn't inlined.  

[more time passes]

And in fact looking at the GCC manual, there's __attribute__((always_inline)), which if we use that says explicitly:

inline-varargs.c:5:1: error: function ‘varargtest’ can never be inlined because it uses variable argument lists
 varargtest (const char *format, ...)
 ^~~~~~~~~~

And a bit more research leads to  __builtin_va_arg_pack() here:
https://gcc.gnu.org/onlinedocs/gcc/Constructing-Calls.html
which is *explicitly* for this case.  But I'm not sure I want to get into the game of finding out how portable that is.  

So I plan to go with a more or less straight copy of the systemd code; I guess the ugly wrinkle

...is handling GNUC varargs vs ISO?

But...apparently MSVC doesn't have statement expressions =( 

https://github.com/AccelerateHS/accelerate/issues/234#issuecomment-70099767
https://visualstudio.uservoice.com/forums/121579-visual-studio-ide/suggestions/15956611-implement-statement-expression-gcc-extension

CLOSED → SCREWED_BY_MSVC ?  

I caved for now and just put it in libglnx: https://github.com/GNOME/libglnx/pull/53

(In reply to Colin Walters from comment #9)
> CLOSED → SCREWED_BY_MSVC ?

For now. :-(