Re: patches for xsl family enhancing bison output

[Akim Demaille]

Hi Jannick,

> Le 27 oct. 2018 à 16:12, Jannick <address@hidden> a écrit :
> 
> Hi Akim,
> 
> please find attached a patch series (6 patches) enhancing the bison output
> in .xml, .html, .text, .dot and .pdf format for your review.
> 
> All patches aim at providing the information about the input grammar file
> path, the date stamp of the bison compilation and the bison version used. I
> found this to be missing in the daily work with bison. The text output
> should be identical regardless if created directly by bison or indirectly by
> applying an xsl to bison's xml output. Same for the dot output. 
> 
> I hope that the implementation and patch documentation meet your and bison's
> requirements, but please amend as you may find appropriate. Same applies to
> the design of the additional header information, of course.

There’s too much work here for me to be able to install them without
you signing the FSF disclaimers.  I’ll put you in too with the person
in charge of that.

I installed 6/6, the spell fix.  It’s simple enough to not need it.

Please, have a look at the git log, and use the same conventions.
In particular, prefer the imperative.  And rework your commits.


> From 892ef0986c99779ec821966c2445f8a747d18da3 Mon Sep 17 00:00:00 2001
> From: Jannick <address@hidden>
> Date: Sat, 27 Oct 2018 13:45:35 +0200
> Subject: [PATCH 2/6] xml-output: add compilation date stamp
> 
> The date time stamp is determined one time only during run time.
> ---
>  src/print-xml.c |  4 ++++
>  src/print.c     | 19 +++++++++++++++++++
>  src/print.h     |  3 ++-
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/print-xml.c b/src/print-xml.c
> index 3f4bc5b..8be81fd 100644
> --- a/src/print-xml.c
> +++ b/src/print-xml.c
> @@ -513,6 +513,10 @@ print_xml (void)
>    xml_printf (out, level + 1, "<filename>%s</filename>",
>                xml_escape (grammar_file));
>  
> +  fputc ('\n', out);
> +  xml_printf (out, level + 1, "<compilation datestamp=\"%s\"/>",
> +              xml_escape (datetimestamp()) );

I’m not sure this is really needed.  The time the file was generated
does not convey a lot of information, does it?  The timestamp of the
original file would make more sense, but still fragile.

Remove the space before the paren.

>    /* print grammar */
>    print_grammar (out, level + 1);
>  
> diff --git a/src/print.c b/src/print.c
> index 36ad9d3..6345ff2 100644
> --- a/src/print.c
> +++ b/src/print.c
> @@ -22,6 +22,7 @@
>  #include "system.h"
>  
>  #include <bitset.h>
> +#include <time.h>
>  
>  #include "LR0.h"
>  #include "closure.h"
> @@ -540,3 +541,21 @@ print_results (void)
>  
>    xfclose (out);
>  }
> +
> +char*
> +datetimestamp ( void )

Follow the same style as the other functions.

> +{
> +  static char *t=NULL;

Follow the same style as elsewhere, please.

> +
> +  if ( !t ) {
> +    char *p;
> +    time_t now;
> +    time ( &now );
> +    t=ctime ( &now );
> +    for ( p=t; p[0] && p[1]; p++ );
> +    while ( t<p && p[0]=='\n' ) {
> +      p--[0]=0;
> +    }

What is going on here?  What is this function doing?  Avoid
loops, see if there is a str* function that does what you
want.  p looks like t + strlen(t) - 1 (except if t == ""
which should not happen).

Instead of making t static, why don’t you make the result static?
Check in gnulib if there’s not a function that does what you want.

ctime seems obsolete.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/ctime.html


> +  }
> +  return t;
> +}
> diff --git a/src/print.h b/src/print.h
> index 61bc205..4e270cb 100644
> --- a/src/print.h
> +++ b/src/print.h
> @@ -20,6 +20,7 @@
>  #ifndef PRINT_H_
>  # define PRINT_H_
>  
> -void print_results (void);
> +void  print_results (void);
> +char* datetimestamp (void);

Don’t change the indentation of print_results.  Make the result const.

>  
>  #endif /* !PRINT_H_ */
> -- 
> 2.19.1.windows.1


Could you please send a few examples of outputs, before and after
your changes?

Thanks!