[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tmpdir.c (path_search_alloc): New function.
From: |
John Darrington |
Subject: |
Re: [PATCH] tmpdir.c (path_search_alloc): New function. |
Date: |
Sun, 13 Sep 2020 14:11:25 +0200 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Sun, Sep 13, 2020 at 01:12:28PM +0200, Bruno Haible wrote:
Hi John,
As this is your first major contribution to Gnulib, can you please make
sure
you have signed a copyright assignment with the FSF regarding 'GNULIB'?
Yes, I have.
> * lib/tmpdir.c (path_search_alloc): Define new function similar to
> path_search, but which allocates the buffer for the result instead
> of relying on the caller to preallocate it.
This is a good idea, because it gets rid of an EINVAL error return.
It also means the caller does not have to guess how long the buffer should be.
> +/* Like path_search, except that TMPL is allocated automatically.
> + TMPL may not be null. *TMPL must be freed by the caller, when no
longer needed.
> + After calling this function *TMPL_LEN will be set to the lenght of
*TMPL. */
> +extern int path_search_alloc (char **tmpl, size_t *tmpl_len, const char
*dir, const char *pfx,
> + bool try_tmpdir);
The calling convention is odd: If the caller is only meant to use *TMPL and
later free() it, why does he need *TMPL_LEN? It seems redundant to return
it
from this function. And then, if *TMPL is the only output (besides the
error
condition), why not make it the return value? That is:
extern char * path_search_alloc (const char *dir, const char *pfx, bool
try_tmpdir);
In case of error, this function would return NULL with errno set.
That would also work. But I don't think the suggested interface is
particularly odd.
It is very similar to the getline function from libc.
Regarding TMPL_LEN, you are right, it *may* not be needed. However I often
find that
when writing code which munges strings, one needs to know the length of a
string which
has already been calculated. Of course one could simply use strlen to find
it, but
strlen is O(n) So I think, that if a function has to calculate a length of a
string
anyway, then it is worthwhile making that number available to the caller.
The caller is free to use it or ignore it as she wishes. Often she'll be
gratefull
for it.
Typo: s/lenght/length/
Thanks for noticing.
Please don't choose function names that start with underscore. These
function
names are in the scope of the vendor libc. And in fact, such function names
would make merging with glibc harder, since glibc already has a function
'__path_search' (defined in sysdeps/posix/tempname.c).
I wondered why that was there.
Also, __path_search is a misnomer now: it does not search anything; it
determines the temporary directory in which to place a temporary file.
So what name would you suggest? "get_temp_directory" ?
J'