[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: Patch: Problem with type safety and the "sentinel" attribute
From:       Stefan Westerfeld <stefan () space ! twc ! de>
Date:       2006-06-19 15:35:25
Message-ID: 20060619153525.GC17197 () space ! twc ! de
[Download RAW message or body]

   Hi!

On Sat, Jun 17, 2006 at 02:59:47PM -0400, Kaveh R. Ghazi wrote:
>  > So the provided patch adds the null_terminated attribute as suggested,
>  > and works as I would expect it to work.
> 
> Sorry, I don't like the name "null_terminated".  It implies two things
> that aren't necessary.  The first is that NULL must be the terminator,
> and second that it must appear as the terminating i.e. "last"
> argument.  Neither is necessarily true and we shouldn't box ourselves
> in with the name.  Look at the design I wrote for attribute sentinel:
> http://gcc.gnu.org/ml/gcc/2004-08/msg01068.html
> 
> In theory it takes two optional arguments, position and value (which
> would also specify type.)  I only implemented the position, but
> nothing stops someone from implementing value/type which has been
> asked for in the past and most recently by Tim in this thread.  So I'd
> like to come up with a name that is neutral on the magic value and
> position.  Maybe "named_sentinel" ?  I'm open to other ideas.
> 
> Note, you don't have to necessarily implement the value/type thing,
> but your design should leave the door open to later backwards
> compatible improvements.

Ok, I see that "sentinel" is the more neutral name. But if you want
every feature of the sentinel attribute to be ported to the
named_sentinel attribute later on, and every feature of the
named_sentinel attribute to be ported to the sentinel attribute later
on, then IMHO we should either merge them completely, or at least ensure
that they will have the same syntax later on.

That is: either we use something like

  __attribute__ ((sentinel ([position [ , named? [ , type [ , value ]]]])));

where

  position is the position from the end of the argument list
  named?   is 0 for the old behaviour and 1 for the behaviour I
           implemented
  type     is one of the three types that Tim suggested (see other mail)
  value    is NULL/0 by default but can be set to -1 or something else
           when necessary

or we use

  __attribute__ ((sentinel       ([position [ , type [ , value ]]])));
  __attribute__ ((named_sentinel ([position [ , type [ , value ]]])));

By the way, it would be possible to make the arguments before the "type"
argument also optional, so that

  __attribute__ ((sentinel (GType)));

would also be valid. But I am not sure whether this is a good idea, or
too confusing in practice.

> For implementation, my suggestion is to copy all of the sentinel code
> as your starting point.  Where the sentinel code skips over the named
> arguments, you could simply stop one parameter earlier.  Or reuse the
> sentinel code by adding a bool parameter to check_function_sentinel
> which says whether to stop on the last named argument before starting
> to search for the sentinel.

I think as the complexity of what you envision is a lot more than what I
thought it would be, the code should be definitely shared, whether there
are two attributes or one.

> You definitely want to check corner cases like the above and your
> patch should include a rigorous testcase.  See the sentinel testcase
> gcc.dg/format/sentinel-1.c for ideas.

Right. I'll add a test. Once it is clear what the code should do
exactly.

   Cu... Stefan
-- 
Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan
[prev in list] [next in list] [prev in thread] [next in thread]