[MPlayer-dev-eng] [PATCH] Parsing simplification in vobsub

[Reimar Döffinger]

On Tue, Apr 13, 2010 at 10:22:18PM +0200, ubitux wrote:
> On Tue, Apr 13, 2010 at 10:05:06PM +0200, Reimar Döffinger wrote:
> > Sorry, that sure makes more sense. Seems ok to me then.
> 
> Right, nice. The full patch is fine too? (reattached to this mail).

It mixes too many different things, particularly the lang/id changes should
be separate, reviewing the in one bunch is too error-prone.
Also I partially retract my previous ok: I think the scanf string does
not exactly match the code.
Also moving all the comments before the function does not really belong in  this
patch.

> +/* id: xx, index: n */
>  static int vobsub_parse_id(vobsub_t *vob, const char *line)
>  {
> -    // id: xx, index: n
> -    size_t idlen;
> -    const char *p, *q;
> -    p  = line;
> -    while (isspace(*p))
> -        ++p;
> -    q = p;
> -    while (isalpha(*q))
> -        ++q;
> -    idlen = q - p;
> -    if (idlen == 0)
> +    char id[3];
> +    unsigned int index;
> +
> +    if (sscanf(line, " %2s, index: %d", id, &index) != 2)
>          return -1;
> -    ++q;
> -    while (isspace(*q))
> -        ++q;
> -    if (strncmp("index:", q, 6))
> -        return -1;
> -    q += 6;
> -    while (isspace(*q))
> -        ++q;
> -    if (!isdigit(*q))
> -        return -1;
> -    return vobsub_add_id(vob, p, idlen, atoi(q));
> +    return vobsub_add_id(vob, id, index);

This does more than just simplify the code, the old code did not
require a comma, any non-alpha character was accepted.
And unless vobsub_add_id had a check for it, it only required
the id to have length > 0, but not to be exactly 2.

> +/* timestamp: HH:MM:SS.mmm, filepos: 0nnnnnnnnn */
>  static int vobsub_parse_timestamp(vobsub_t *vob, const char *line)
>  {
> -    // timestamp: HH:MM:SS.mmm, filepos: 0nnnnnnnnn
> -    const char *p;
>      int h, m, s, ms;
>      off_t filepos;
> -    while (isspace(*line))
> -        ++line;
> -    p = line;
> -    while (isdigit(*p))
> -        ++p;
> -    if (p - line != 2)
> +
> +    if (sscanf(line, " %d:%d:%d:%d , filepos: %lx ", &h, &m, &s, &ms, &filepos) != 5)

Doesn't match either, old code required time values to have exactly length
2, 2, 2 and 3, no whitespace was allowed before the ",", an %lx
is not generally the correct format specified for off_t (you could just
use uint64_t and PRIx64).
And the terminating whitespace at best just wastes memory.

> +/* org: X, Y */
>  static int vobsub_parse_origin(vobsub_t *vob, const char *line)
>  {
> -    // org: X,Y
> -    char *p;
> -    while (isspace(*line))
> -        ++line;
> -    if (!isdigit(*line))
> -        return -1;
> -    vob->origin_x = strtoul(line, &p, 10);
> -    if (*p != ',')
> -        return -1;
> -    ++p;
> -    vob->origin_y = strtoul(p, NULL, 10);
> -    return 0;
> +    unsigned int x, y;
> +
> +    if (sscanf(line, " %d , %d", &x, &y) == 2) {

No whitespace was allowed before the comma originally.