Re: [PATCH] Rewrite iconv option parsing [BZ #19519]

[Arjun Shankar <arjun dot is at lostca dot se>]

On Wed, Dec 18, 2019 at 12:18:10PM +0100, Florian Weimer wrote:
> 
> > +/* This function returns a pointer to the last suffix in a conversion code
> > +   string.  Valid suffixes matched by this function are of the form: '/' or ','
> > +   followed by arbitrary text that doesn't contain '/' or ','.  It does not
> > +   edit the string in any way.  The caller is expected to parse the suffix and
> > +   advance the string terminator before the next call.  */
> > +static char *
> > +find_suffix (char *s)
> > +{
> > +  /* The conversion code is in the form of a triplet, separated by '/' chars.
> > +     The third component of the triplet contains suffixes. If we don't have two
> > +     slashes, we don't have a suffix.  */
> > +
> > +  int slash_count = 0;
> > +
> > +  for (int i = 0; s[i] != '\0'; i++)
> > +    if (s[i] == '/')
> > +      slash_count++;
> > +
> > +  if (slash_count < 2)
> > +    return NULL;
> 
> If the caller advances the string, as described in the function comment,
> does this still work?  Won't slash_count drop below 2, causing suffixes
> to be ignored?

I used a less than ideal word, "advance", to describe what happens to the
string terminator. It moves back. i.e. suffixes are found via find_suffix,
then dropped before the next call.

Call 1: "FOO/BAR/HACK,FROB"
                     ^
                     |
                     suffix

Call 2: "FOO/BAR/HACK"
                ^
                |
                suffix

Call 3: "FOO/BAR" (too few '/' characters => no suffix)

I can make the comment clearer by dropping the line:

"The caller is expected to parse the suffix and advance the string terminator
before the next call."

...and replacing it with:

"The caller is expected to parse the suffix and remove it from the string (by
e.g. terminating the string earlier) before the next call."

> > +             && (isspace(pc->code[len - 1])
> 
> Missing space after isspace.

Thanks! I'll add one.