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

[Stefan Westerfeld <stefan space twc de>]

   Hi!

On Fri, Jun 09, 2006 at 07:30:25PM +0200, Tim Janik wrote:
> 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.

For the statistics, by adding the __sentinel__ attribute to the beast
codebase, I have about 50 of those sentinel warnings that I don't need.
So the double NULL termination would make quite some code more messy
than it should be. 

> >> 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.

Ah, I see.

> >> 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.

I also like the backwards compatible way better than trying to somehow
modifying the attribute; it may break something now, or - which would
also be bad - it may make something that somebody will want to check
with the sentinel attribute in some future program impossible.

The only case that I ever needed was NULL termination, so I think
implementing one of the two proposals Tim made would be sufficient.
Personally I like __attribute__((__null_terminated__)) better, because
a -1 sentinel may be less intuitive to read in the source code. So this
would be my suggestion for a "syntax specification".

> >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 ;)

Good. Of course I would be happy if somebody with knowledge of the
compiler source could implement it. I never hacked gcc code before.
But since you suggested sending a patch, I'll at least try to implement
a new __null_terminated__ attribute, and ask for help if I can't figure
out what to do.

   Cu... Stefan
-- 
Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan