Re: [PATCH 2/4] errors: show carets

[Akim Demaille]

Le 4 déc. 2012 à 15:26, Theophile Ranquet <address@hidden> a écrit :

> diff --git a/src/location.c b/src/location.c
> index fa1b53c..3c3bc6e 100644
> --- a/src/location.c
> +++ b/src/location.c
> @@ -138,6 +138,77 @@ location_print (FILE *out, location loc)
>   return res;
> }
> 
> +struct caret_info
> +{
> +  FILE* source;
> +  size_t line;
> +  size_t offset;
> +};

Please, comment all this (structure, and what follows).

> +
> +static struct caret_info caret_info = { NULL, 1, 0 };
> +
> +void
> +cleanup_caret ()
> +{
> +  if (caret_info.source)
> +    fclose (caret_info.source);
> +}
> +
> +void
> +location_caret (FILE *out, location loc)
> +{
> +  int i;

Please, use shorter scopes for this 'i'.

> +  char buf[1024];
> +
> +  /* FIXME: find a way to support X-file locations, and only open once each
> +     file. That would make the procedure future-proof.  */
> +  if (! (caret_info.source
> +         || (caret_info.source = fopen (loc.start.file, "r")))

Don't you have to fclose before fopen?

> +      || loc.start.column > sizeof buf)
> +    return;
> +
> +  /* If the line we want to quote is seekable (the same line as the previous
> +     location), just seek it. If it was before, we lost track of it, so
> +     return to the start of file.  */
> +  if (caret_info.line <= loc.start.line)
> +    fseek (caret_info.source, caret_info.offset, SEEK_SET);
> +  else
> +    {
> +      caret_info.line = 1, caret_info.offset = 0;

Two stmts please.

> +      fseek (caret_info.source, caret_info.offset, SEEK_SET);
> +    }
> +
> +  /* Advance to the line's position, keeping track of the offset.  */
> +  for (i = caret_info.line; i < loc.start.line; caret_info.offset++)
> +    if (fgetc (caret_info.source) == '\n')
> +      ++i;
> +  caret_info.line = loc.start.line;
> +
> +  /* Read the actual line.  Don't update the offset, so that we keep a 
> pointer
> +     to the start of the line.  */
> +  if (fgets (buf, sizeof buf, caret_info.source))
> +    {
> +      size_t len = strlen (buf);

fgets will not support \0.  Can't we use getline instead?
It should also address some limitations of this implementation
(1024 for instance, although I agree that at some point in
width, caret diagnostics are no longer useful).

> +      /* The caret of a multiline location ends with the first line.  */
> +      int end = loc.start.line != loc.end.line ? len : loc.end.column;
> +
> +      if (len)
> +        {
> +          /* Quote the file, by displaying the read line prefixed and
> +             suffixed with the indentation, here 1 character.  */
> +          buf[len - 1] = '\n';
> +          fprintf (out, " %s ", buf);
> +
> +          /* Print the caret.  */
> +          for (i = 1; i < loc.start.column; ++i)
> +            fprintf (out, " ");

fprintf (out, "%*s", loc.start.column - 1, "");

> +          for (; i < end || i == loc.start.column; ++i)
> +            fprintf (out, "^");

putc should suffice.

> +        }
> +      fprintf (out, "\n");

Ditto.

> +    }
> +}
> +
> void
> boundary_set_from_string (boundary *bound, char *loc_str)
> {
> diff --git a/src/location.h b/src/location.h
> index 5ebb92e..3bd44be 100644
> --- a/src/location.h
> +++ b/src/location.h
> @@ -102,6 +102,11 @@ void location_compute (location *loc,
>    characters.  */
> unsigned location_print (FILE *out, location loc);
> 
> +void cleanup_caret (void);

Please, doc.

> +
> +/* Output to OUT the line and caret corresponding to location LOC.  */
> +void location_caret (FILE *out, location loc);
> +
> /* Return -1, 0, 1, depending whether a is before, equal, or
>    after b.  */
> static inline int
> diff --git a/src/main.c b/src/main.c
> index 0396b0f..184d789 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -216,5 +216,7 @@ main (int argc, char *argv[])
>   timevar_stop (TV_TOTAL);
>   timevar_print (stderr);
> 
> +  cleanup_caret ();
> +
>   return complaint_issued ? EXIT_FAILURE : EXIT_SUCCESS;
> }
> -- 
> 1.8.0
>