Re: Problem with type safety and the "sentinel" attribute

[Tim Janik <timj imendio com>]

thanks for the quick response Kaveh.

On Fri, 9 Jun 2006, Kaveh R. Ghazi wrote:

> >  void print_string_array (const char *array_name,
> >                                 const char *string, ...) __attribute__
> >                                 ((__sentinel__));
> >
> >   print_string_array ("empty_array", NULL); /* gcc warns, but shouldn't */
> >
> > The only way out for keeping the sentinel attribute and avoiding the
> > warning is using
> >
> > static void print_string_array (const char *array_name, ...)
> > __attribute__ ((__sentinel__));
>
> I think you could maintain typesafety and silence the warning by
> keeping the more specific prototype and adding an extra NULL, e.g.:
>
> print_string_array ("empty_array", NULL, NULL);
>
> Doesn't seem elegant, but it does the job.

this is an option for a limited set of callers, yes.

> > By the way, there is already an existing gcc bug, which is about the
> > same thing (NULL passed within named args), but wants to have it the
> > way it works now:
> >
> >   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21911
>
> Correct, the feature as I envisioned it expected the sentinel to
> appear in the variable arguments only.  This PR reflects when I found
> out it didn't do that and got around to fixing it.  Note the "buggy"
> behavior wasn't exactly what you wanted either because GCC got fooled
> by a sentinel in *any* of the named arguments, not just the last one.
>
>
> > so if it gets changed, then gcc might need to support both
> >  - NULL termination within the last named parameter allowed
> >  - NULL termination only allowed within varargs parameters (like it is
> >    now)
>
> I'm not against this enhancement, but you need to specify a syntax
> that allows the old behavior but also allows doing it your way.
>
> Hmm, perhaps we could check for attribute "nonnull" on the last named
> argument, if it exists then that can't be the sentinel, if it's not
> there then it does what you want.  This is not completely backwards
> compatible because anyone wanting the existing behavior has to add the
> attribute nonnull.  But there's precedent for this when attribute
> printf split out whether the format specifier could be null...
>
> We could also create a new attribute name for the new behavior.  This
> would preserve backwards compatibility.  I like this idea better.

i agree here. as far as the majority of the GLib and Gtk+ APIs are concerned,
we don't really need the flexibility of the sentinel attribute but rather
a compiler check on whether the last argument used in a function call
is NULL or 0 (regardless of whether it's the last named arg or already part
of the varargs list).
that's also why the actual sentinel wrapper in GLib looks like this:

  #define G_GNUC_NULL_TERMINATED __attribute__((__sentinel__))

so, if i was to make a call on this issue, i'd either introduce
__attribute__((__null_terminated__)) with the described semantics,
or have __attribute__((__sentinel__(-1))) work essentially like
__attribute__((__sentinel__(0))) while also accepting 0 in the position
of the last named argument.

> Next you need to recruit someone to implement this enhancement, or
> submit a patch. :-) Although given that you can silence the warning by
> adding an extra NULL at the call site, I'm not sure it's worth it.

i would say this is definitely worth it, because the issue also shows up in
other code that is widely used:
  gpointer    g_object_new       (GType           object_type,
                                  const gchar    *first_property_name,
                                                  ...);
that's for instance a function which is called in many projects. 
putting the burden on the caller is clearly the wrong trade off here.

so please take this as a vote for the worthiness of a fix ;)

> 		--Kaveh
> --
> Kaveh R. Ghazi			ghazi caip rutgers edu

---
ciaoTJ