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 report!

> […]
> Second problem: when "bison -Werror" turns a warning into an error, it
> still outputs a parser.
> In a makefile, this causes the following scenario:
> 
> % make
> [bison signals an error and build stops]
> % make
> [build continue without running bison again]
> 
> I'm unhappy with that, because unless I touch the parser again or
> clean my project,
> I will have no chance to realize I still have some error to fix.
> 
> I'm not sure what the correct behaviour should be here.  Should Bison be 
> changed
> to not create the output file on error (like gcc), or should Makefiles always 
> do
>    bison -Wall -Werror parse.yy -o parser.tmp && mv parser.tmp parser.cc
> ?

Eventually I would really like to have a system for temporary files
that would ensure that the generation is atomic.  This would protect
from bad C-c.


I have addressed your concerns in the following series of patches
for the maint branch.  They seem to fix your problem.  I plan to
release 3.0.2 soon, with these changes included.

Cheers!

        Akim

commit 184b42c85be3d42c958173c550ea442baf96a8cd
Author: Akim Demaille <address@hidden>
Date:   Wed Dec 4 09:30:08 2013 +0100

    output: do not generate source files when early errors are caught
    
    Reported by Alexandre Duret-Lutz as "second problem" in:
    http://lists.gnu.org/archive/html/bug-bison/2013-09/msg00015.html
    
    One problem is that some errors are caught early, before the
    generation of output files, while others can only be detected
    afterwards (since, for instance, skeletons can raise errors
    themselves).
    
    This will be addressed in two steps: early errors do not generate
    source files at all, while later errors will remove the files that
    have already been generated.
    
    * src/scan-skel.l (yyout): Open to /dev/null when there are errors.
    * tests/output.at (AT_CHECK_FILES): Factored out of...
    (AT_CHECK_OUTPUT): this.
    Fuse the "SHELLIO" argument in the "FLAGS" one.
    Use $5 to denote the expected exit status.
    Add a test case for early errors.

diff --git a/NEWS b/NEWS
index 1fa7d9b..fdd9248 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,16 @@ GNU Bison NEWS
 
 ** Bug fixes
 
+*** Generated source files when errors are reported
+
+  When warnings are issued and -Werror is set, bison would still generate
+  the source files (*.c, *.h...).  As a consequence, some runs of "make"
+  could fail the first time, but not the second (as the files were generated
+  anyway).
+
+  This is fixed: bison no longer generates this source files, but, of
+  course, still produces the various reports (*.output, *.xml, etc.).
+
 *** %empty is used in reports
 
   Empty right-hand sides are denoted by '%empty' in all the reports (text,
diff --git a/src/scan-skel.l b/src/scan-skel.l
index f13ee81..48c5e46 100644
--- a/src/scan-skel.l
+++ b/src/scan-skel.l
@@ -245,7 +245,8 @@ at_output (int argc, char *argv[], char **out_namep, int 
*out_linenop)
     }
   *out_namep = xstrdup (argv[1]);
   output_file_name_check (out_namep);
-  yyout = xfopen (*out_namep, "w");
+  /* If there were errors, do not generate the output.  */
+  yyout = xfopen (complaint_status ? "/dev/null" : *out_namep, "w");
   *out_linenop = 1;
 }
 
diff --git a/tests/output.at b/tests/output.at
index 266a503..66a3e5e 100644
--- a/tests/output.at
+++ b/tests/output.at
@@ -17,12 +17,23 @@
 
 AT_BANNER([[Output file names.]])
 
+# AT_CHECK_FILES(EXPECTED-FILES, [IGNORED-FILES])
+# -----------------------------------------------
+# Check that the current directory contains FILE... (sorted).
+m4_define([AT_CHECK_FILES],
+[AT_CHECK([[find . -type f |
+           $PERL -ne '
+      s,\./,,; chomp;
+      push @file, $_ unless m{^($2|testsuite.log)$};
+      END { print join (" ", sort @file), "\n" }']],
+          [], [$1
+])])
 
-# AT_CHECK_OUTPUT(INPUT-FILE, [DIRECTIVES], [FLAGS], EXPECTED-FILES, [SHELLIO],
+# AT_CHECK_OUTPUT(INPUT-FILE, [DIRECTIVES], [FLAGS], EXPECTED-FILES, [STATUS],
 #                 [ADDITIONAL-TESTS], [PRE-TESTS])
 # -----------------------------------------------------------------------------
 m4_define([AT_CHECK_OUTPUT],
-[AT_SETUP([[Output files: ]$2 $3 $5])[
+[AT_SETUP([[Output files: ]$2 $3])[
 ]$7[
 for file in ]$1 $4[; do
   case $file in
@@ -35,15 +46,9 @@ done
 foo: {};
 ]])[
 
-]AT_BISON_CHECK([$3 $1 $5], 0)[
+]AT_BISON_CHECK([$3 $1], [$5], [], [ignore])[
 # Ignore the files non-generated files
-]AT_CHECK([[find . -type f |
-           $PERL -ne '
-      s,\./,,; chomp;
-      push @file, $_ unless m{^($1|testsuite.log)$};
-      END { print join (" ", sort @file), "\n" }']],
-          [], [$4
-])[
+]AT_CHECK_FILES([$4], [$1])[
 ]$6[
 ]AT_CLEANUP[
 ]])
@@ -54,9 +59,9 @@ AT_CHECK_OUTPUT([foo.y], [], [-dv],
 # Some versions of Valgrind (at least valgrind-3.6.0.SVN-Debian) report
 # "fgrep: write error: Bad file descriptor" when stdout is closed, so we
 # skip this test group during maintainer-check-valgrind.
-AT_CHECK_OUTPUT([foo.y], [], [-dv],
+AT_CHECK_OUTPUT([foo.y], [], [-dv >&-],
                 [foo.output foo.tab.c foo.tab.h],
-                [>&-], [],
+                [], [],
                 [AT_CHECK([[case "$PREBISON" in *valgrind*) exit 77;; esac]])])
 
 AT_CHECK_OUTPUT([foo.y], [], [-dv -o foo.c],
@@ -114,6 +119,12 @@ AT_CHECK_OUTPUT([foo.yy], [],
                 [-o foo.c++ --graph=foo.gph],
                 [foo.c++ foo.gph])
 
+# Do not generate code when there are early errors (even warnings as
+# errors).
+AT_CHECK_OUTPUT([foo.y], [%type <foo> useless],
+                [--defines --graph --xml --report=all -Wall -Werror],
+                [foo.dot foo.output foo.xml],
+                [1])
 
 ## ------------ ##
 ## C++ output.  ##

commit ea99d6e6a02a06bd63e788393fd42cde5cb1fa71
Author: Akim Demaille <address@hidden>
Date:   Wed Dec 4 10:26:46 2013 +0100

    output: record what generated files are source or report files
    
    * src/files.h, src/files.c (output_file_name_check): Take an additional
    argument to record whether a file is a source or report file.
    * src/files.c (generated_file): New.
    (file_names, file_names_count): Replace with...
    (generated_files, generated_files_size): these.
    * src/scan-skel.l: Adjust.

diff --git a/src/files.c b/src/files.c
index 76aa7fe..4686836 100644
--- a/src/files.c
+++ b/src/files.c
@@ -51,8 +51,17 @@ char *spec_defines_file = NULL;  /* for --defines. */
 char *parser_file_name;
 
 /* All computed output file names.  */
-static char **file_names = NULL;
-static int file_names_count = 0;
+typedef struct generated_file
+{
+  /** File name.  */
+  char *name;
+  /** Whether is a generated source file (e.g., *.c, *.java...), as
+      opposed to the report file (e.g., *.output).  When late errors
+      are detected, generated source files are removed.  */
+  bool is_source;
+} generated_file;
+static generated_file *generated_files = NULL;
+static int generated_files_size = 0;
 
 uniqstr grammar_file = NULL;
 uniqstr current_file = NULL;
@@ -332,21 +341,21 @@ compute_output_file_names (void)
     {
       if (! spec_graph_file)
         spec_graph_file = concat2 (all_but_tab_ext, ".dot");
-      output_file_name_check (&spec_graph_file);
+      output_file_name_check (&spec_graph_file, false);
     }
 
   if (xml_flag)
     {
       if (! spec_xml_file)
         spec_xml_file = concat2 (all_but_tab_ext, ".xml");
-      output_file_name_check (&spec_xml_file);
+      output_file_name_check (&spec_xml_file, false);
     }
 
   if (report_flag)
     {
       if (!spec_verbose_file)
         spec_verbose_file = concat2 (all_but_tab_ext, OUTPUT_EXT);
-      output_file_name_check (&spec_verbose_file);
+      output_file_name_check (&spec_verbose_file, false);
     }
 
   free (all_but_tab_ext);
@@ -355,7 +364,7 @@ compute_output_file_names (void)
 }
 
 void
-output_file_name_check (char **file_name)
+output_file_name_check (char **file_name, bool source)
 {
   bool conflict = false;
   if (STREQ (*file_name, grammar_file))
@@ -367,11 +376,11 @@ output_file_name_check (char **file_name)
   else
     {
       int i;
-      for (i = 0; i < file_names_count; i++)
-        if (STREQ (file_names[i], *file_name))
+      for (i = 0; i < generated_files_size; i++)
+        if (STREQ (generated_files[i].name, *file_name))
           {
             complain (NULL, Wother, _("conflicting outputs to file %s"),
-                      quote (*file_name));
+                      quote (generated_files[i].name));
             conflict = true;
           }
     }
@@ -382,9 +391,10 @@ output_file_name_check (char **file_name)
     }
   else
     {
-      file_names = xnrealloc (file_names, ++file_names_count,
-                              sizeof *file_names);
-      file_names[file_names_count-1] = xstrdup (*file_name);
+      generated_files = xnrealloc (generated_files, ++generated_files_size,
+                                   sizeof *generated_files);
+      generated_files[generated_files_size-1].name = xstrdup (*file_name);
+      generated_files[generated_files_size-1].is_source = source;
     }
 }
 
@@ -400,8 +410,8 @@ output_file_names_free (void)
   free (dir_prefix);
   {
     int i;
-    for (i = 0; i < file_names_count; i++)
-      free (file_names[i]);
+    for (i = 0; i < generated_files_size; i++)
+      free (generated_files[i].name);
   }
-  free (file_names);
+  free (generated_files);
 }
diff --git a/src/files.h b/src/files.h
index ebe5037..94833f7 100644
--- a/src/files.h
+++ b/src/files.h
@@ -63,7 +63,12 @@ extern char *all_but_ext;
 
 void compute_output_file_names (void);
 void output_file_names_free (void);
-void output_file_name_check (char **file_name);
+
+/** Record that we generate file \a file_name.
+ *  \param source whether this is a source file (*c, *.java...)
+ *                as opposed to a report (*.output, *.dot...).
+ */
+void output_file_name_check (char **file_name, bool source);
 
 FILE *xfopen (const char *name, char const *mode);
 void xfclose (FILE *ptr);
diff --git a/src/scan-skel.l b/src/scan-skel.l
index 48c5e46..129b889 100644
--- a/src/scan-skel.l
+++ b/src/scan-skel.l
@@ -244,7 +244,7 @@ at_output (int argc, char *argv[], char **out_namep, int 
*out_linenop)
       xfclose (yyout);
     }
   *out_namep = xstrdup (argv[1]);
-  output_file_name_check (out_namep);
+  output_file_name_check (out_namep, true);
   /* If there were errors, do not generate the output.  */
   yyout = xfopen (complaint_status ? "/dev/null" : *out_namep, "w");
   *out_linenop = 1;

commit 461983270c01b28cbb73424535408c8aded31573
Author: Akim Demaille <address@hidden>
Date:   Wed Dec 4 10:51:47 2013 +0100

    output: do not generate source files when late errors are caught
    
    Reported by Alexandre Duret-Lutz as "second problem" in:
    http://lists.gnu.org/archive/html/bug-bison/2013-09/msg00015.html
    
    * bootstrap.conf: We need the "unlink" module.
    * src/files.h, src/files.c (unlink_generated_sources): New.
    * src/output.c: Use it.
    * tests/output.at: Check the case of late errors.

diff --git a/bootstrap.conf b/bootstrap.conf
index c58470e..186afa7 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -34,7 +34,8 @@ gnulib_modules='
   readme-release
   realloc-posix
   spawn-pipe stdbool stpcpy strdup-posix strerror strtoul strverscmp
-  unistd unistd-safer unlocked-io update-copyright unsetenv verify
+  unistd unistd-safer unlink unlocked-io
+  update-copyright unsetenv verify
   warnings
   xalloc
   xalloc-die
diff --git a/src/files.c b/src/files.c
index 4686836..c50f774 100644
--- a/src/files.c
+++ b/src/files.c
@@ -399,6 +399,16 @@ output_file_name_check (char **file_name, bool source)
 }
 
 void
+unlink_generated_sources (void)
+{
+  int i;
+  for (i = 0; i < generated_files_size; i++)
+    if (generated_files[i].is_source)
+      /* Ignore errors.  The file might not even exist.  */
+      unlink (generated_files[i].name);
+}
+
+void
 output_file_names_free (void)
 {
   free (all_but_ext);
diff --git a/src/files.h b/src/files.h
index 94833f7..9b85719 100644
--- a/src/files.h
+++ b/src/files.h
@@ -70,6 +70,9 @@ void output_file_names_free (void);
  */
 void output_file_name_check (char **file_name, bool source);
 
+/** Remove all the generated source files. */
+void unlink_generated_sources (void);
+
 FILE *xfopen (const char *name, char const *mode);
 void xfclose (FILE *ptr);
 FILE *xfdopen (int fd, char const *mode);
diff --git a/src/output.c b/src/output.c
index 5eafb2e..ab1bdea 100644
--- a/src/output.c
+++ b/src/output.c
@@ -704,6 +704,11 @@ output (void)
   /* Process the selected skeleton file.  */
   output_skeleton ();
 
+  /* If late errors were generated, destroy the generated source
+     files. */
+  if (complaint_status)
+    unlink_generated_sources ();
+
   obstack_free (&format_obstack, NULL);
 }
 
diff --git a/tests/output.at b/tests/output.at
index 66a3e5e..be3078f 100644
--- a/tests/output.at
+++ b/tests/output.at
@@ -43,7 +43,7 @@ done
 ]AT_DATA([$1],
 [$2[
 %%
-foo: {};
+foo: %empty {};
 ]])[
 
 ]AT_BISON_CHECK([$3 $1], [$5], [], [ignore])[
@@ -126,6 +126,14 @@ AT_CHECK_OUTPUT([foo.y], [%type <foo> useless],
                 [foo.dot foo.output foo.xml],
                 [1])
 
+# Do not generate code when there are late errors (even warnings as
+# errors).
+AT_CHECK_OUTPUT([foo.y], [%define useless],
+                [--defines --graph --xml --report=all -Wall -Werror],
+                [foo.dot foo.output foo.xml],
+                [1])
+
+
 ## ------------ ##
 ## C++ output.  ##
 ## ------------ ##