On Thu, 22 Dec 2005, Tim Jenness wrote:

> On Thu, 22 Dec 2005, Mark Taylor wrote:

> > > I've been thinking about the "datGetVC" problem over breakfast. How about
> > > I change the definition of "maxval" to be the declared size of pntrs[]?
> > > Then I can check that pntrs[] is large enough to hold the null and set
> > > status bad if it isn't. Then the caller would know there was a problem and
> > > be able to fix the code without overwriting random memory.
> >
> > I retain a philosophical uneasiness on the grounds that I don't think
> > calls to a low-level library like HDS should return the same
> > information in more than one form. Perhaps though I shouldn't be
>
> What do you mean? I'm happy to hear your opinion on the API given that it
> must be C like but also given that most people want to be responsible for
> allocating their own buffer. The reason there is a character buffer and an
> array of pointers is because that was deemed to be the easiest way to get
> the char** array without relying on the library doing lots of mallocing
> that could easily lead to memory leaks (at least without providing a
> helper function for freeing all the memory in the strings).

I agree with all this; the user should be responsible for the mallocing.
However, what I understand you to be proposing is a function which
retains the same prototype as now, though with modified semantics:

   datGetVC( const HDSLoc * locator,
             size_t maxval,
             size_t bufsize,
             char *buffer,
             char *pntrs[],
             size_t * actval,
             int * status );

You then have the information about how long the returned buffer is
in two forms:

   1. the value returned in actval
   2. the position of the NULL at the end of buffer

I don't see that item (2) gives you much in the way of useful information;
it does allow you to write

   for ( ; *buffer; buffer++ ) f( *buffer );

but you could write

   for ( ; actval >= 0; actval-- ) f( buffer[actval] );

in any case, so it doesn't buy you much (unless I've missed the
point here). Personally the fewer decisions I have to make about
how to code something the happier I am, though this is a matter of
taste, hence my TMTOWTDI comment.

So I'm suggesting that you either do what you say with changing the
semantics of maxval but also drop the actval argument, or retain
actval and forget the NULL termination. My preferred preference
would be for the latter (since you might want actval up front to
do some additional allocation say).


> Are you against the API as a whole or just the extra NULL in the pointer
> array?

Just the latter. All the rest of it looks fine, and a great improvement
on using CNF.

Mark

--
Mark Taylor Astronomical Programmer Physics, Bristol University, UK
[log in to unmask] +44-117-928-8776 http://www.star.bris.ac.uk/~mbt/