[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tmpdir.c (path_search_alloc): New function.
From: |
Bruno Haible |
Subject: |
Re: [PATCH] tmpdir.c (path_search_alloc): New function. |
Date: |
Sun, 13 Sep 2020 16:07:15 +0200 |
User-agent: |
KMail/5.1.3 (Linux/4.4.0-189-generic; KDE/5.18.0; x86_64; ; ) |
Hi John,
> > +/* 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.
The 'getline' function is not a good model to imitate. Why? Because generally
there
should be two supported ways to call such a function
(a) with a NULL argument - to let the function allocate as much memory as it
needs,
(b) with a stack-allocated buffer as argument - to let the function use that
buffer
if its size is sufficient.
The 'getline' function, as documented [1], does not support the second case.
Its calling convention is thus apparently tailored to the use-case of calling
it in
a loop, avoiding memory allocations if a line is shorter than the previous line.
But this is a *very* special use-case, and most functions that people write —
including
path_search_alloc — are not like this.
Actually your function supports (a) and (b). But the documentation is lacking
and
incorrect:
- "TMPL may not be null." OK but what can the caller put in TMPL?
- "*TMPL must be freed by the caller, when no longer needed." Not true.
In case (b), *TMPL is unchanged, and thus must not be free()d.
How about using this wording, from the GNU libunistring documentation [2]:
[Functions returning a string result] take a (resultbuf, lengthp) argument
pair. If resultbuf is not NULL and the result fits into *lengthp units, it
is put in resultbuf, and resultbuf is returned. Otherwise, a freshly
allocated string is returned. In both cases, *lengthp is set to the length
of the returned string. In case of error, NULL is returned and errno is set.
And the prototype that goes with it:
extern char * path_search_alloc (char *resultbuf, size_t *lengthp,
const char *dir, const char *pfx, bool
try_tmpdir);
This is simpler than
extern int path_search_alloc (char **tmpl, size_t *tmpl_len,
const char *dir, const char *pfx, bool
try_tmpdir);
in two aspects:
- It has one less indirection.
- In the use-cases (a) and (b) the caller has one less local variable.
> 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)
OK, but then make sure that the caller understands what the returned value is.
If you call it 'tmpl_len' everyone would assume that tmpl_len == strlen (tmpl).
But in fast you return strlen (tmpl) + 1. Therefore it should be called
'tmpl_size', not 'tmpl_len'.
> 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" ?
How about renaming
path_search -> gen_tempfile_template_prealloc
path_search_alloc -> gen_tempfile_template_alloc
? The first one takes preallocated storage, whereas your function allocates
storage.
Bruno
[1] https://linux.die.net/man/3/getline
[2] https://www.gnu.org/software/libunistring/manual/html_node/Conventions.html