Re: [PATCH v2] Associate target_ops with target_fileio file descriptors

[Gary Benson <gbenson@redhat.com>]

Pedro Alves wrote:
> On 03/20/2015 04:06 PM, Gary Benson wrote:
> > +
> > +  /* Search for closed handles to reuse.  */
> > +  if (lowest_closed_fd >= 0)
> > +    {
> > +      for (index = lowest_closed_fd;
> > +	   VEC_iterate (fileio_fh_t, fileio_fhandles, index, fh);
> > +	   index++)
> > +	if (is_closed_fileio_fh (fh->fd))
> > +	  break;
> 
> You don't really need 'index'.  If this walks using
> lowest_closed_fd instead, like:
> 
>       for (;
> 	   VEC_iterate (fileio_fh_t, fileio_fhandles,
>                         lowest_closed_fd, fh);
> 	   lowest_closed_fd++)
> 	if (is_closed_fileio_fh (fh->fd))
> 	  break;
> 
> Then the next time around we skip slots we already know are
> open.
> 
> > +    }
> > +
> > +  /* Push a new handle if no closed handles were found.  */
> > +  if (index == total)
> > +    {
> > +      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
> > +      lowest_closed_fd = -1;
> > +    }
> > +
> > +  /* Fill in the handle and return its index.  */
> > +  fh->t = t;
> > +  fh->fd = fd;
> > +
> > +  return index;
> > +}

You do need another variable though.  The function returns the
index of the file handle it just used, so you need to save that
across the bit that sets lowest_closed_fd to -1.  I rearranged
things to this, but it's longer and I'd argue harder to follow:

  /* Acquire a target fileio file descriptor.  */
  
  static int
  acquire_fileio_fd (struct target_ops *t, int fd)
  {
    int total = VEC_length (fileio_fh_t, fileio_fhandles);
    int index = total;
    fileio_fh_t *fh, buf;
  
    gdb_assert (!is_closed_fileio_fh (fd));
  
    /* Search for closed handles to reuse.  */
    if (lowest_closed_fd >= 0)
      {
        for (;
             VEC_iterate (fileio_fh_t, fileio_fhandles,
                          lowest_closed_fd, fh);
             lowest_closed_fd++)
          if (is_closed_fileio_fh (fh->fd))
            break;
  
        index = lowest_closed_fd;
      }
  
    /* Push a new handle if no closed handles were found.  */
    if (index == total)
      {
        fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
        lowest_closed_fd = -1;
      }
  
    /* Fill in the handle and return its index.  */
    fh->t = t;
    fh->fd = fd;
  
    return index;
  }

Do you prefer that?

I noticed you can do "lowest_closed_fd++" if you don't create a new
handle, so whichever way you like I'll add that micro-optimization.

Cheers,
Gary

--
http://gbenson.net/