bug-gnulib
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]