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
