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

[Théophile Ranquet]

2012/12/4 Akim Demaille <address@hidden>:
>> +struct caret_info
>> +{
>> +  FILE* source;
>> +  size_t line;
>> +  size_t offset;
>> +};
>
> Please, comment all this (structure, and what follows).

I have commented this, in both the .c and the .h files.

>> +  /* 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?

If a file has been previously opened, the pointer won't be null, and
thus no new file will be opened. The fclose is done "manually" by
calling cleanup_caret (). We do not currently support locations over
multiple files, so I don't think the need to change / close files
midway will occur anytime soon. Please tell me if I am wrong in
assuming this.

I have taken into consideration your other comments, and installed the
following:


commit 3f5d1b2c67651a9d620946de421f2e51600b885e
Author: Theophile Ranquet <address@hidden>
Date:   Fri Nov 30 14:34:56 2012 +0100

    errors: show carets

    * src/locations.c (caret_info): New, persistant information useful
    for...
    (location_caret): New, print a caret.
    (cleanup_caret): Release caret_info cleanly, call it...
    * src/main.c (main): Here.
    * src/complain.c (error_message): Call location_caret here.

diff --git a/src/complain.c b/src/complain.c
index b063c6b..ede0ccf 100644
--- a/src/complain.c
+++ b/src/complain.c
@@ -74,11 +74,15 @@ error_message (location *loc,
   vfprintf (stderr, message, args);
   {
     size_t l = strlen (message);
-    if (l < 2 || message[l-2] != ':' || message[l-1] != ' ') {
-      putc ('\n', stderr);
-      fflush (stderr);
-    }
+    if (l < 2 || message[l - 2] != ':' || message[l - 1] != ' ')
+      {
+        putc ('\n', stderr);
+        fflush (stderr);
+        if (loc && feature_flag & feature_caret)
+          location_caret (stderr, *loc);
+      }
   }
+  fflush (stderr);
 }

 /** Wrap error_message() with varargs handling. */
diff --git a/src/getargs.c b/src/getargs.c
index 1b7c6f1..c8865bf 100644
--- a/src/getargs.c
+++ b/src/getargs.c
@@ -646,7 +646,7 @@ getargs (int argc, char *argv[])
        exit (EXIT_SUCCESS);

       case 'f':
-        FLAGS_ARGMATCH (flag, optarg);
+        FLAGS_ARGMATCH (feature, optarg);
         break;

       case 'W':
diff --git a/src/gram.c b/src/gram.c
index d1b3804..5730e59 100644
--- a/src/gram.c
+++ b/src/gram.c
@@ -308,11 +308,16 @@ grammar_rules_useless_report (const char *message)
   for (r = 0; r < nrules ; ++r)
     if (!rules[r].useful)
       {
-        warn_at (rules[r].location, "%s: ", message);
-        if (warnings_flag & warnings_other)
+        if (feature_flag & feature_caret)
+          warn_at (rules[r].location, "%s", message);
+        else
           {
-            rule_print (&rules[r], stderr);
-            fflush (stderr);
+            warn_at (rules[r].location, "%s: ", message);
+            if (warnings_flag & warnings_other)
+              {
+                rule_print (&rules[r], stderr);
+                fflush (stderr);
+              }
           }
       }
 }
diff --git a/src/location.c b/src/location.c
index fa1b53c..6e5306c 100644
--- a/src/location.c
+++ b/src/location.c
@@ -138,6 +138,89 @@ location_print (FILE *out, location loc)
   return res;
 }

+
+/* Persistant data used by location_caret to avoid reopening and rereading the
+   same file all over for each error.  */
+struct caret_info
+{
+  FILE* source;
+  size_t line;
+  size_t offset;
+};
+
+static struct caret_info caret_info = { NULL, 1, 0 };
+
+/* Free any allocated ressources and close any open file handles that are
+   left-over by the usage of location_caret.  */
+void
+cleanup_caret ()
+{
+  if (caret_info.source)
+    fclose (caret_info.source);
+}
+
+/* Output to OUT the line and caret corresponding to location LOC.  */
+void
+location_caret (FILE *out, location loc)
+{
+  /* 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")))
+      || loc.start.column == -1 || loc.start.line == -1)
+    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;
+      fseek (caret_info.source, caret_info.offset, SEEK_SET);
+    }
+
+  /* Advance to the line's position, keeping track of the offset.  */
+  {
+    int i;
+    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.  */
+  {
+    ssize_t len = 0;
+    char *buf = NULL;
+    if ((len = getline (&buf, (size_t*) &len, caret_info.source)) != -1)
+      {
+        /* 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)
+          {
+            int i = loc.start.column;
+            /* Quote the file, indent by a single column.  */
+            fputc (' ', out);
+            fwrite (buf, 1, len, out);
+
+            /* Print the caret, with the same indent as above.  */
+            fputc (' ', out);
+            fprintf (out, "%*s", loc.start.column - 1, "");
+            do {
+              fputc ('^', out);
+            } while (++i < end);
+          }
+        fputc ('\n', out);
+        free (buf);
+      }
+  }
+}
+
 void
 boundary_set_from_string (boundary *bound, char *loc_str)
 {
diff --git a/src/location.h b/src/location.h
index 5ebb92e..c1859ae 100644
--- a/src/location.h
+++ b/src/location.h
@@ -102,6 +102,13 @@ void location_compute (location *loc,
    characters.  */
 unsigned location_print (FILE *out, location loc);

+/* Free any allocated ressources and close any open file handles that are
+   left-over by the usage of location_caret.  */
+void cleanup_caret (void);
+
+/* 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;
 }