Re: [PATCH 3/6] warnings: organize variadic complaints call

[Akim Demaille]

Le 1 oct. 2012 à 17:01, Theophile Ranquet a écrit :

> Move the dispatch of variadic complains to complain.c, rather
> than do it in a scanner. This is more appropriate.
> 
> * src/complain.c (complain_var): Move (and refactor) contents here...
> (complain_atp): New.
> * src/complain.h: Adjust,
> * src/scan-skel.l (at_directive_perform): ... from here.

This is completely wrong, mentioning functions that
do not exist.


I have installed a quite different patch, as there are several
issues in this patch:


> +/* This function is used to convert calls to va_list args functions from
> +   an argv[] model */
> +void
> +complain_args (warnings w, location const *locp, int argc, char *argv[])

Document the prototype of public functions, in the header.
Not to mention that, for consistency, the arguments should
be reversed.

> --- a/src/scan-skel.l
> +++ b/src/scan-skel.l
> @@ -173,6 +173,7 @@ flag (const char *arg)
>     case 'w': return Wother;
>     case 'c': return complaint;
>     case 'f': return fatal;
> +    case 'o': return Wnone;
>     default: aver (false); break;
>     }
> }
> @@ -186,82 +187,40 @@ at_directive_perform (int argc, char *argv[], char 
> **outnamep, int *out_linenop)
>         fail_for_at_directive_too_many_args (argv[0]);
>       fputs (last_component (argv[1]), yyout);
>     }
> -  else if (STREQ (argv[0], "@warn")
> -           || STREQ (argv[0], "@complain")
> -           || STREQ (argv[0], "@fatal"))
> +  else
>     {
> -      warnings w = flag (argv[0]);
> -      switch (argc)
> -        {
> -          case 2:
> -            complain (NULL, w, "%s", _(argv[1]));
> -            break;
> -          case 3:
> -            complain (NULL, w, _(argv[1]), argv[2]);
> -            break;
> -          case 4:
> -            complain (NULL, w, _(argv[1]), argv[2], argv[3]);
> -            break;
> -          case 5:
> -            complain (NULL, w, _(argv[1]), argv[2], argv[3], argv[4]);
> -            break;
> -          case 6:
> -            complain (NULL, w, _(argv[1]), argv[2], argv[3], argv[4], 
> argv[5]);
> -            break;
> -          default:
> -            fail_for_at_directive_too_many_args (argv[0]);
> -            break;
> +      warnings w = flag (*argv);
> +      if (w & (Wother | complaint | fatal))

This considerably weakens the tests performed, so I
wrote something different.

I also had to disable a function which is no longer used.

Please, try to factor at the M4 level too: there are
too many @commands to issue warnings, we don't need
that many.  For a start, use empty location instead
of foo_at vs. foo.

This is what I installed:

commit 782e8187185a009ce698ad294c87e49a0a30c3a1
Author: Theophile Ranquet <address@hidden>
Date:   Mon Oct 1 15:01:00 2012 +0000

    warnings: organize variadic complaints call
    
    Move the dispatch of variadic complains to complain.c, rather than do
    it in a scanner.
    
    * src/complain.h, src/complain.c (complain_args): New.
    * src/scan-skel.l (at_directive_perform): Use it.
    
    Signed-off-by: Akim Demaille <address@hidden>

diff --git a/src/complain.c b/src/complain.c
index 1d43f1d..ddd4156 100644
--- a/src/complain.c
+++ b/src/complain.c
@@ -161,3 +161,29 @@ complain_at_indent (location loc, warnings flags, unsigned 
*indent,
   complains (&loc, flags, message, args);
   va_end (args);
 }
+
+void
+complain_args (location const *loc, warnings w, int argc, char *argv[])
+{
+  switch (argc)
+  {
+  case 2:
+    complain (loc, w, "%s", _(argv[1]));
+    break;
+  case 3:
+    complain (loc, w, _(argv[1]), argv[2]);
+    break;
+  case 4:
+    complain (loc, w, _(argv[1]), argv[2], argv[3]);
+    break;
+  case 5:
+    complain (loc, w, _(argv[1]), argv[2], argv[3], argv[4]);
+    break;
+  case 6:
+    complain (loc, w, _(argv[1]), argv[2], argv[3], argv[4], argv[5]);
+    break;
+  default:
+    complain (loc, fatal, "too many arguments for complains");
+    break;
+  }
+}
diff --git a/src/complain.h b/src/complain.h
index 31084e0..809f1b4 100644
--- a/src/complain.h
+++ b/src/complain.h
@@ -61,6 +61,9 @@ void warnings_print_categories (warnings warn_flags);
 void complain (location const* loc, warnings flags, char const *message, ...)
   __attribute__ ((__format__ (__printf__, 3, 4)));
 
+/** Likewise, but with an \a argc/argv interface.  */
+void complain_args (location const *loc, warnings w, int argc, char *arg[]);
+
 /** Make a complaint with location and some indentation.  */
 void complain_at_indent (location loc, warnings flags, unsigned *indent,
                          char const *message, ...)
diff --git a/src/scan-skel.l b/src/scan-skel.l
index 9854727..68f528c 100644
--- a/src/scan-skel.l
+++ b/src/scan-skel.l
@@ -186,65 +186,26 @@ at_directive_perform (int argc, char *argv[], char 
**outnamep, int *out_linenop)
         fail_for_at_directive_too_many_args (argv[0]);
       fputs (last_component (argv[1]), yyout);
     }
-  else if (STREQ (argv[0], "@warn")
-           || STREQ (argv[0], "@complain")
-           || STREQ (argv[0], "@fatal"))
+  else if (STREQ (argv[0], "@warn") || STREQ (argv[0], "@warn_at")
+           || STREQ (argv[0], "@complain") || STREQ (argv[0], "@complain_at")
+           || STREQ (argv[0], "@fatal") || STREQ (argv[0], "@fatal_at"))
     {
-      warnings w = flag (argv[0]);
-      switch (argc)
-        {
-          case 2:
-            complain (NULL, w, "%s", _(argv[1]));
-            break;
-          case 3:
-            complain (NULL, w, _(argv[1]), argv[2]);
-            break;
-          case 4:
-            complain (NULL, w, _(argv[1]), argv[2], argv[3]);
-            break;
-          case 5:
-            complain (NULL, w, _(argv[1]), argv[2], argv[3], argv[4]);
-            break;
-          case 6:
-            complain (NULL, w, _(argv[1]), argv[2], argv[3], argv[4], argv[5]);
-            break;
-          default:
-            fail_for_at_directive_too_many_args (argv[0]);
-            break;
-        }
-    }
-  else if (STREQ (argv[0], "@warn_at")
-           || STREQ (argv[0], "@complain_at")
-           || STREQ (argv[0], "@fatal_at"))
-    {
-      warnings w = flag (argv[0]);
+      warnings w = flag (*argv);
       location loc;
-      if (argc < 4)
-        fail_for_at_directive_too_few_args (argv[0]);
-      boundary_set_from_string (&loc.start, argv[1]);
-      boundary_set_from_string (&loc.end, argv[2]);
-      switch (argc)
+      location *locp = NULL;
+      if (STREQ (*argv + strlen (*argv) - 3, "_at"))
         {
-          case 4:
-            complain (&loc, w, "%s", _(argv[3]));
-            break;
-          case 5:
-            complain (&loc, w, _(argv[3]), argv[4]);
-            break;
-          case 6:
-            complain (&loc, w, _(argv[3]), argv[4], argv[5]);
-            break;
-          case 7:
-            complain (&loc, w, _(argv[3]), argv[4], argv[5], argv[6]);
-            break;
-          case 8:
-            complain (&loc, w, _(argv[3]), argv[4], argv[5], argv[6],
-                         argv[7]);
-            break;
-          default:
-            fail_for_at_directive_too_many_args (argv[0]);
-            break;
+          if (argc < 4)
+            fail_for_at_directive_too_few_args (argv[0]);
+          boundary_set_from_string (&loc.start, argv[1]);
+          boundary_set_from_string (&loc.end, argv[2]);
+          argc -= 2;
+          argv += 2;
+          locp = &loc;
         }
+      else if (argc < 2)
+        fail_for_at_directive_too_few_args (argv[0]);
+      complain_args (locp, w, argc, argv);
     }
   else if (STREQ (argv[0], "@output"))
     {