Re: two -Werror issues

[Akim Demaille]

Le 30 sept. 2013 à 11:42, Alexandre Duret-Lutz <address@hidden> a écrit :

> Hi,

Hi Alexandre,

Thanks for the detailed bug report.

> % bison -Wall -Werror --report=all
> -Wno-error=empty-rule,no-error=deprecated ltlparse.yy -o ltlparse.cc
> ltlparse.yy:323.17: error: empty rule without %empty [-Werror=empty-rule]
> OP_SQBKT_SEP_opt: | OP_SQBKT_SEP_unbounded
>                 ^
> ltlparse.yy:324.10: error: empty rule without %empty [-Werror=empty-rule]
> error_opt: | error
>          ^
> ltlparse.yy:31.9-25: error: %define variable 'api.location.type'
> requires '{...}' values [-Werror=deprecated]
> %define api.location.type "spot::location"
>         ^^^^^^^^^^^^^^^^^
> 
> But these warnings are still reported as errors.
> (-Wno-empty-rule,no-deprecated will work by simply
> silencing the warning, but this is less satisfactory.)

This is unfortunate, indeed.

It turned out to be less easy to fix than I first thought.
Actually, I had to "lâcher du lest" (give leeway) on something
I thought important: if -Werror or -Wno-error is last, it applies
to all the previous options.

It turns out that's not what compilers do, so I sticked to their
behavior, which is "if something specific was specified, it takes
precedence over -W(no-)error, independently of their relative
positions".

So I installed the following in the maint branch.  Thanks again.

commit 2b7fe38c36cdf69428039c4bafbf47be15b807fa
Author: Akim Demaille <address@hidden>
Date:   Tue Oct 8 13:01:24 2013 +0200

    comment changes
    
    * src/complain.h, src/complain.c: More documentation, more comments.

diff --git a/src/complain.c b/src/complain.c
index fdc4b54..d2f3f4a 100644
--- a/src/complain.c
+++ b/src/complain.c
@@ -38,11 +38,11 @@ bool warnings_are_errors = false;
 /** Diagnostics severity.  */
 typedef enum
   {
-    severity_disabled = 0,
-    severity_unset = 1,
-    severity_warning = 2,
-    severity_error = 3,
-    severity_fatal = 4
+    severity_disabled = 0, /**< Explicitly disabled via -Wno-foo.  */
+    severity_unset = 1,    /**< Unspecified status.  */
+    severity_warning = 2,  /**< A warning.  */
+    severity_error = 3,    /**< An error (continue, but die soon).  */
+    severity_fatal = 4     /**< Fatal error (die now).  */
   } severity;
 
 
@@ -151,7 +151,10 @@ warnings_argmatch (char *args)
         }
       else
         {
+          // The length of the possible 'no-' prefix: 3, or 0.
           size_t no = STRPREFIX_LIT ("no-", args) ? 3 : 0;
+          // The length of the possible 'error=' (possibly after
+          // 'no-') prefix: 6, or 0.
           size_t err = STRPREFIX_LIT ("error=", args + no) ? 6 : 0;
 
           warning_argmatch (args, no, err);
diff --git a/src/complain.h b/src/complain.h
index 0d81503..62dd8fa 100644
--- a/src/complain.h
+++ b/src/complain.h
@@ -128,14 +128,14 @@ void deprecated_directive (location const *loc,
 void duplicate_directive (char const *directive,
                           location first, location second);
 
-/** Warnings treated as errors shouldn't stop the execution as regular errors
-    should (because due to their nature, it is safe to go on). Thus, there are
-    three possible execution statuses.  */
+/** Warnings treated as errors shouldn't stop the execution as regular
+    errors should (because due to their nature, it is safe to go
+    on). Thus, there are three possible execution statuses.  */
 typedef enum
   {
-    status_none,
-    status_warning_as_error,
-    status_complaint
+    status_none,             /**< No diagnostic issued so far.  */
+    status_warning_as_error, /**< A warning was issued (but no error).  */
+    status_complaint         /**< An error was issued.  */
   } err_status;
 
 /** Whether an error was reported.  */

commit e4678430c2506c577c05e92437b187fe9daf0b7f
Author: Akim Demaille <address@hidden>
Date:   Wed Oct 16 10:55:28 2013 +0200

    diagnostics: "-Werror -Wno-error=foo" must not emit errors
    
    Currently "-Werror -Wno-error=foo" still turns "foo" warnings into errors.
    Reported by Alexandre Duret-Lutz.
    See http://lists.gnu.org/archive/html/bug-bison/2013-09/msg00015.html.
    
    * src/complain.c (errority, errority_flag): New.
    (complain_init): Initialize the latter.
    (warning_argmatch): Extract the loop iterating on the flag's bits.
    Set and unset errority_flag here.
    (warnings_argmatch): -Wno-error is not the same as -Wno-error=everything:
    we must remember if category foo was explicitly turned in an error/warning
    via -W(no-)error=foo.
    (warning_severity): Use errority_flag.
    
    * tests/input.at (Symbols): Just check --yacc, not -Wyacc, that's the
    job of tests on -W.
    (-Werror is not affected by -Wnone and -Wall): Rename as...
    (-Werror combinations): this.
    Tests more combinations of -W, -W(no-)error, and -W(no-)error=foo.
    * tests/local.at (AT_BISON_CHECK_WARNINGS): Don't expect -Werror
    to turn runs that issue warnings into runs with errors, as the
    warnings might be enforced as warnings by -Wno-error=foo, in which
    case -Werror does not change anything.
    
    * doc/bison.texi (Bison Options): Try to be clearer about how
    -W(no-)error and -W(no-)error=foo interact.

diff --git a/NEWS b/NEWS
index bbdcc0f..7a2d4b5 100644
--- a/NEWS
+++ b/NEWS
@@ -4,7 +4,16 @@ GNU Bison NEWS
 
 ** Bug fixes
 
-  Portability issues in the test suite.
+*** Portability issues in the test suite.
+
+*** Fixes of the -Werror option
+
+  Options such as "-Werror -Wno-error=foo" were still turning "foo"
+  diagnostics into errors instead of warnings.  This is fixed.
+
+  Actually, for consistency with GCC, "-Wno-error=foo -Werror" now also
+  leaves "foo" diagnostics as warnings.  Similarly, with "-Werror=foo
+  -Wno-error", "foo" diagnostics are now errors.
 
 * Noteworthy changes in release 3.0 (2013-07-25) [stable]
 
diff --git a/doc/bison.texi b/doc/bison.texi
index 78d7d06..cd5e440 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -10065,18 +10065,16 @@ A category can be turned off by prefixing its name 
with @samp{no-}.  For
 instance, @option{-Wno-yacc} will hide the warnings about
 POSIX Yacc incompatibilities.
 
address@hidden address@hidden
address@hidden address@hidden
-Enable warnings falling in @var{category}, and treat them as errors.  If no
address@hidden is given, it defaults to making all enabled warnings into errors.
address@hidden -Werror
+Turn enabled warnings for every @var{category} into errors, unless they are
+explicitly disabled by @address@hidden
+
address@hidden address@hidden
+Enable warnings falling in @var{category}, and treat them as errors.
 
 @var{category} is the same as for @option{--warnings}, with the exception that
 it may not be prefixed with @samp{no-} (see above).
 
-Prefixed with @samp{no}, it deactivates the error treatment for this
address@hidden However, the warning itself won't be disabled, or enabled, by
-this option.
-
 Note that the precedence of the @samp{=} and @samp{,} operators is such that
 the following commands are @emph{not} equivalent, as the first will not treat
 S/R conflicts as errors.
@@ -10086,6 +10084,14 @@ $ bison -Werror=yacc,conflicts-sr input.y
 $ bison -Werror=yacc,error=conflicts-sr input.y
 @end example
 
address@hidden -Wno-error
+Do not turn enabled warnings for every @var{category} into errors, unless
+they are explicitly enabled by @address@hidden
+
address@hidden address@hidden
+Deactivate the error treatment for this @var{category}. However, the warning
+itself won't be disabled, or enabled, by this option.
+
 @item -f address@hidden
 @itemx address@hidden
 Activate miscellaneous @var{feature}. @var{feature} can be one of:
diff --git a/src/complain.c b/src/complain.c
index d2f3f4a..115b704 100644
--- a/src/complain.c
+++ b/src/complain.c
@@ -35,6 +35,17 @@ err_status complaint_status = status_none;
 
 bool warnings_are_errors = false;
 
+/** Whether -Werror/-Wno-error was applied to a warning.  */
+typedef enum
+  {
+    errority_unset = 0,     /** No explict status.  */
+    errority_disabled = 1,  /** Explictly disabled with -Wno-error=foo.  */
+    errority_enabled = 2    /** Explictly enabled with -Werror=foo. */
+  } errority;
+
+/** For each warning type, its errority.  */
+static errority errority_flag[warnings_size];
+
 /** Diagnostics severity.  */
 typedef enum
   {
@@ -103,32 +114,26 @@ warning_argmatch (char const *arg, size_t no, size_t err)
       no = !no;
     }
 
-  if (no)
-    {
-      size_t b;
-      for (b = 0; b < warnings_size; ++b)
-        if (value & 1 << b)
+  size_t b;
+  for (b = 0; b < warnings_size; ++b)
+    if (value & 1 << b)
+      {
+        if (err && no)
+          /* -Wno-error=foo.  */
+          errority_flag[b] = errority_disabled;
+        else if (err && !no)
           {
-            if (err)
-              {
-                /* -Wno-error=foo: if foo enabled as an error,
-                   make it a warning.  */
-                if (warnings_flag[b] == severity_error)
-                  warnings_flag[b] = severity_warning;
-              }
-            else
-              /* -Wno-foo.  */
-              warnings_flag[b] = severity_disabled;
+            /* -Werror=foo: enables -Wfoo. */
+            errority_flag[b] = errority_enabled;
+            warnings_flag[b] = severity_warning;
           }
-    }
-  else
-    {
-      size_t b;
-      for (b = 0; b < warnings_size; ++b)
-        if (value & 1 << b)
-          /* -Wfoo and -Werror=foo. */
-          warnings_flag[b] = err ? severity_error : severity_warning;
-    }
+        else if (no)
+          /* -Wno-foo.  */
+          warnings_flag[b] = severity_disabled;
+        else
+          /* -Wfoo. */
+          warnings_flag[b] = severity_warning;
+      }
 }
 
 /** Decode a comma-separated list of arguments from -W.
@@ -145,10 +150,7 @@ warnings_argmatch (char *args)
       if (STREQ (args, "error"))
         warnings_are_errors = true;
       else if (STREQ (args, "no-error"))
-        {
-          warnings_are_errors = false;
-          warning_argmatch ("no-error=everything", 3, 6);
-        }
+        warnings_are_errors = false;
       else
         {
           // The length of the possible 'no-' prefix: 3, or 0.
@@ -176,27 +178,46 @@ complain_init (void)
 
   size_t b;
   for (b = 0; b < warnings_size; ++b)
-    warnings_flag[b] = (1 << b & warnings_default
-                        ? severity_warning
-                        : severity_unset);
+    {
+      warnings_flag[b] = (1 << b & warnings_default
+                          ? severity_warning
+                          : severity_unset);
+      errority_flag[b] = errority_unset;
+    }
 }
 
+
+/* A diagnostic with FLAGS is about to be issued.  With what severity?
+   (severity_fatal, severity_error, severity_disabled, or
+   severity_warning.) */
+
 static severity
 warning_severity (warnings flags)
 {
   if (flags & fatal)
+    /* Diagnostics about fatal errors.  */
     return severity_fatal;
   else if (flags & complaint)
+    /* Diagnostics about errors.  */
     return severity_error;
   else
     {
+      /* Diagnostics about warnings.  */
       severity res = severity_disabled;
       size_t b;
       for (b = 0; b < warnings_size; ++b)
         if (flags & 1 << b)
-          res = res < warnings_flag[b] ? warnings_flag[b] : res;
-      if (res == severity_warning && warnings_are_errors)
-        res = severity_error;
+          {
+            res = res < warnings_flag[b] ? warnings_flag[b] : res;
+            /* If the diagnostic is enabled, and -Werror is enabled,
+               and -Wno-error=foo was not explicitly requested, this
+               is an error. */
+            if (res == severity_warning
+                && (errority_flag[b] == errority_enabled
+                    || (warnings_are_errors
+                        && errority_flag[b] != errority_disabled)))
+              res = severity_error;
+          }
       return res;
     }
 }
diff --git a/tests/input.at b/tests/input.at
index eb73bf9..4ddf955 100644
--- a/tests/input.at
+++ b/tests/input.at
@@ -956,15 +956,9 @@ without_period: "WITHOUT.PERIOD";
 AT_BISON_OPTION_POPDEFS
 
 # POSIX Yacc accept periods, but not dashes.
-AT_BISON_CHECK([--yacc -Wno-error input.y], [], [],
-[[input.y:9.8-16: warning: POSIX Yacc forbids dashes in symbol names: 
WITH-DASH [-Wyacc]
-input.y:20.8-16: warning: POSIX Yacc forbids dashes in symbol names: with-dash 
[-Wyacc]
-]])
-
-# So warn about them.
-AT_BISON_CHECK([-Wyacc input.y], [], [],
-[[input.y:9.8-16: warning: POSIX Yacc forbids dashes in symbol names: 
WITH-DASH [-Wyacc]
-input.y:20.8-16: warning: POSIX Yacc forbids dashes in symbol names: with-dash 
[-Wyacc]
+AT_BISON_CHECK([--yacc input.y], [1], [],
+[[input.y:9.8-16: error: POSIX Yacc forbids dashes in symbol names: WITH-DASH 
[-Werror=yacc]
+input.y:20.8-16: error: POSIX Yacc forbids dashes in symbol names: with-dash 
[-Werror=yacc]
 ]])
 
 # Dashes are fine for GNU Bison.
@@ -1768,11 +1762,11 @@ AT_BISON_CHECK([[-Dparse.lac.memory-trace=full 
input.y]],
 
 AT_CLEANUP
 
-## --------------------------------------------- ##
-## -Werror is not affected by -Wnone and -Wall.  ##
-## --------------------------------------------- ##
+## ---------------------- ##
+## -Werror combinations.  ##
+## ---------------------- ##
 
-AT_SETUP([[-Werror is not affected by -Wnone and -Wall]])
+AT_SETUP([[-Werror combinations]])
 
 AT_DATA([[input.y]],
 [[%%
@@ -1798,6 +1792,18 @@ AT_BISON_CHECK([[-Werror,no-all,other input.y]], [[1]], 
[[]],
 [[input.y:2.15: error: stray '$' [-Werror=other]
 ]])
 
+# Check that -Wno-error keeps warnings enabled, but non fatal.
+AT_BISON_CHECK([[-Werror -Wno-error=other input.y]], [[0]], [[]],
+[[input.y:2.15: warning: stray '$' [-Wother]
+]])
+
+AT_BISON_CHECK([[-Wno-error=other -Werror input.y]], [[0]], [[]],
+[[input.y:2.15: warning: stray '$' [-Wother]
+]])
+
+AT_BISON_CHECK([[-Werror=other -Wno-other input.y]], [[0]], [[]],
+[[]])
+
 AT_CLEANUP
 
 
diff --git a/tests/local.at b/tests/local.at
index 7948faa..ee00e00 100644
--- a/tests/local.at
+++ b/tests/local.at
@@ -635,9 +635,12 @@ m4_define([AT_BISON_CHECK_],
 # ----------------------------------------------------------
 # Check that warnings (if some are expected) are correctly
 # turned into errors with -Werror, etc.
+#
+# When -Wno-error is used, the rules are really different, don't try.
 m4_define([AT_BISON_CHECK_WARNINGS],
 [m4_if(m4_bregexp([$4], [: warning: ]), [-1], [],
-      [m4_null_if([$2], [AT_BISON_CHECK_WARNINGS_($@)])])])
+       m4_bregexp([$1], [-Wno-error=]), [-1],
+                  [m4_null_if([$2], [AT_BISON_CHECK_WARNINGS_($@)])])])
 
 m4_define([AT_BISON_CHECK_WARNINGS_],
 [[# Defining POSIXLY_CORRECT causes bison to complain if options are