Re: [PATCH 1/3] grep: add --warnings={always,never,auto}.

[Jim Meyering]

Paolo Bonzini wrote:
> * src/grep.h (no_warnings): New declaration.
> * src/main.c (no_warnings): New.
> (WARNINGS_OPTION): Add to enum.
> (main): Add --warnings.  Handle color_option == 2 together with it.

Hi Paolo,

Thanks for writing that.
Here are some suggestions.
This patch affects mostly 1/3, but the s/no_warnings/warnings/ name
change also affects 3/3.

  - it's almost always better to avoid using a variable name that starts
    with "no_"; use the name "warnings", not "no_warnings".  Best to keep
    the logic "positive", and to avoid double-negatives like "!no_warnings".

  - I prefer --warn= over --warnings=, since the shorter form is a verb,
    and that works slightly better with the adverb values like "never"
    and "always"

  - I've added names for the values 0,1,2.  Using names makes it easier
    to read the resulting code.  That meant having two variables, a new
    enum one (named "w") that's only in main.c, and the global/extern
    boolean (int-but-should-be-bool -- left it for now, for consistency)
    that's set in main.c and used in dfasearch.c.

  - using XARGMATCH provides two benefits: it saves several lines and
    enables abbreviated RHS values, e.g., --warn=alw, which is consistent
    with the way other such options are handled (e.g., ls --color=...
    and cp --sparse=...)

If the change set below is ok with you, let me know and I'll be happy to
amend and push.

One other nit that I did not address:

    I have a fundamental aversion to making functionality like this new
    warning depend on whether a standard input or output stream is a TTY.
    Personally, I want my script to fail if I use a regexp that deserves
    a warning.  Obviously, having grep generate a diagnostic might be
    enough, but might not make a difference at all.
    I'd rather not have script/command-line stderr differ by default.

I noticed a diagnostic that can be improved:

diff --git a/src/dfa.c b/src/dfa.c
index 19a6746..486458c 100644
--- a/src/dfa.c
+++ b/src/dfa.c
@@ -1032,7 +1032,7 @@ parse_bracket_exp (void)
          (c = c1) != ']'));

   if (colon_warning_state == 7)
-    dfawarn ("character classes syntax is [[:space:]], not [:space:]");
+    dfawarn ("character class syntax is [[:space:]], not [:space:]");

 #if MBS_SUPPORT
   if (MB_CUR_MAX > 1

I was surprised that making that change does not trigger a test failure.
It should, but there's no big hurry.

Jim

>From c9f7c1dda6dae243f1fb2416995fc214eed9e322 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 27 Aug 2010 11:39:36 +0200
Subject: [PATCH] .

---
 src/dfasearch.c |    2 +-
 src/grep.h      |    2 +-
 src/main.c      |   63 ++++++++++++++++++++++++++++++++++---------------------
 3 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/src/dfasearch.c b/src/dfasearch.c
index bc2bb2f..29a1f7e 100644
--- a/src/dfasearch.c
+++ b/src/dfasearch.c
@@ -48,7 +48,7 @@ static size_t pcount;
 void
 dfawarn (char const *mesg)
 {
-  if (!no_warnings)
+  if (warnings)
     error (0, 0, _("warning: %s"), mesg);
 }

diff --git a/src/grep.h b/src/grep.h
index 868037e..64c1865 100644
--- a/src/grep.h
+++ b/src/grep.h
@@ -44,4 +44,4 @@ extern int match_words;               /* -w */
 extern int match_lines;                /* -x */
 extern unsigned char eolbyte;  /* -z */

-extern int no_warnings;
+extern int warnings;
diff --git a/src/main.c b/src/main.c
index 36002a0..5314912 100644
--- a/src/main.c
+++ b/src/main.c
@@ -281,7 +281,7 @@ enum
   EXCLUDE_DIRECTORY_OPTION,
   GROUP_SEPARATOR_OPTION,
   MMAP_OPTION,
-  WARNINGS_OPTION,
+  WARN_OPTION,
 };

 /* Long options equivalences. */
@@ -337,7 +337,7 @@ static struct option const long_options[] =
   {"binary", no_argument, NULL, 'U'},
   {"unix-byte-offsets", no_argument, NULL, 'u'},
   {"version", no_argument, NULL, 'V'},
-  {"warnings", optional_argument, NULL, WARNINGS_OPTION},
+  {"warn", optional_argument, NULL, WARN_OPTION},
   {"with-filename", no_argument, NULL, 'H'},
   {"word-regexp", no_argument, NULL, 'w'},
   {0, 0, 0, 0}
@@ -348,14 +348,13 @@ int match_icase;
 int match_words;
 int match_lines;
 unsigned char eolbyte;
+int warnings;

 /* For error messages. */
 /* The name the program was run with, stripped of any leading path. */
 static char const *filename;
 static int errseen;

-int no_warnings;
-
 enum directories_type
   {
     READ_DIRECTORIES = 2,
@@ -383,6 +382,27 @@ static enum
     SKIP_DEVICES
   } devices = READ_DEVICES;

+enum warn_type
+  {
+    WARN_ALWAYS = 0,
+    WARN_NEVER,
+    WARN_AUTO,
+  };
+
+static char const *const warn_args[] =
+{
+  "always", "yes", "force",
+  "never", "no", "none",
+  "auto", "tty", "if-tty", NULL
+};
+static enum warn_type const warn_types[] =
+{
+  WARN_ALWAYS, WARN_ALWAYS, WARN_ALWAYS,
+  WARN_NEVER, WARN_NEVER, WARN_NEVER,
+  WARN_AUTO, WARN_AUTO, WARN_AUTO
+};
+ARGMATCH_VERIFY (warn_args, warn_types);
+
 static int grepdir (char const *, struct stats const *);
 #if defined HAVE_DOS_FILE_CONTENTS
 static inline int undossify_input (char *, size_t);
@@ -1757,6 +1777,7 @@ main (int argc, char **argv)
   int opt, cc, status;
   int default_context;
   FILE *fp;
+  enum warn_type w;

   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
@@ -1789,7 +1810,8 @@ main (int argc, char **argv)
   exit_failure = EXIT_TROUBLE;
   atexit (close_stdout);

-  no_warnings = getenv ("POSIXLY_CORRECT") != NULL;
+  w = getenv ("POSIXLY_CORRECT") ? WARN_NEVER : WARN_ALWAYS;
+
   prepend_default_options (getenv ("GREP_OPTIONS"), &argc, &argv);
   setmatcher (NULL);

@@ -2070,21 +2092,11 @@ main (int argc, char **argv)
         /* long options */
         break;

-      case WARNINGS_OPTION:
-        if(optarg) {
-          if(!strcasecmp(optarg, "always") || !strcasecmp(optarg, "yes") ||
-             !strcasecmp(optarg, "force"))
-            no_warnings = 0;
-          else if(!strcasecmp(optarg, "never") || !strcasecmp(optarg, "no") ||
-                  !strcasecmp(optarg, "none"))
-            no_warnings = 1;
-          else if(!strcasecmp(optarg, "auto") || !strcasecmp(optarg, "tty") ||
-                  !strcasecmp(optarg, "if-tty"))
-            no_warnings = 2;
-          else
-            show_help = 1;
-        } else
-          no_warnings = 0;
+      case WARN_OPTION:
+        if (optarg)
+          w = XARGMATCH ("--warn", optarg, warn_args, warn_types);
+        else
+          w = WARN_ALWAYS;
         break;

       default:
@@ -2115,8 +2127,8 @@ main (int argc, char **argv)
     {
       if (color_option == 2)
         color_option = 1;
-      if (no_warnings == 2)
-        no_warnings = 0;
+      if (w == WARN_AUTO)
+        w = WARN_ALWAYS;
     }
   else
     {
@@ -2124,10 +2136,13 @@ main (int argc, char **argv)
         color_option = 0;

       /* Redirected stdout: we're likely in a script, disable warnings.  */
-      if (no_warnings == 2)
-        no_warnings = 1;
+      if (w == WARN_AUTO)
+        w = WARN_NEVER;
     }

+  /* assert (w == WARN_ALWAYS || w == WARN_NEVER); */
+  warnings = w == WARN_ALWAYS;
+
   if (color_option)
     {
       /* Legacy.  */
--
1.7.2.2.510.g7180a