Re: [Pspp-cvs] pspp po/en_GB.po po/pspp.pot src/data/ChangeLog...

[Ben Pfaff]

I like the idea of adding more ds_*() functions but I have a few
comments on the details.

John Darrington <address@hidden> writes:

> +/* Removes leading spaces from ST.
> +   Returns number of spaces removed. */
> +int
> +ds_ltrim_spaces (struct string *st) 
> +{
> +  int idx = ds_n_find(st, "\t ");
> +  if (0 == idx)
> +    return 0;
> +
> +  if (idx < 0 ) 
> +    {
> +      int len = ds_length(st);
> +      ds_clear(st);
> +      return len;
> +    }

It might be simpler to just write 
   if (idx < 0)
     idx = ds_length (st);
and then let ds_replace() handle the rest.

> +  ds_replace(st, &ds_c_str(st)[idx]);
> +    
> +  return idx;
> +}
> +
> +


> +int
> +ds_find(const struct string *st, const char cs[])

I've been trying to name ds_*() functions after the
corresponding str*() functions, where there's one that's
analogous.  So I'd think that ds_strspn() or ds_span() would be a
good name for this function.  ds_find() makes me think of
strstr() or strchr().

It might be better to return ds_length (st) when the string
consists entirely of characters in cs, as does strspn().  Then
you wouldn't even need a special case in ds_ltrim_spaces().

> +/* Returns the index of the first character in ST which 
> +   is NOT an element of the set CS.
> +   Returns -1 if no such character is found.
> +*/
> +int
> +ds_n_find(const struct string *st, const char cs[])

Similarly, I would name this function something like ds_cspan()
and change the return value semantics.
-- 
"It was then I realized how dire my medical situation was.  Here I was,
 a network admin, unable to leave, and here was someone with a broken
 network.  And they didn't ask me to fix it.  They didn't even try to
 casually pry a hint out of me." --Ryan Tucker in the Monastery