diff mbox

gengtype improvements for plugins, completed! patch 2/N [verbosity]

Message ID 20100908175108.GA1112@hector.lesours
State New
Headers show

Commit Message

Basile Starynkevitch Sept. 8, 2010, 5:51 p.m. UTC 
Hello All,

[join work by Basile Starynkevitch & Jeremie Salvucci]

References: http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02060.html
http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00616.html
http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00663.html

The second piece [verbosity] of our serie of patches adds verbosity to
gengtype. The gengtype user (either a plugin developer or a true GCC
hacker) can pass the -v program flag. This flag can even be passed
more than once, to increase the verbosity level. Verbose messages
explain succintly to the user what gengtype is actually doing.  We
feel this functionality is useful for plugin developpers (who might
not understand what gengtype is doing today, since it is essentially
silent) and even for GCC hackers.

We did took into account Laurynas comments from
http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00210.html

In addition of verbose messages, we added a tiny but useful feature to
gengtype. When it is generating a file gt-foo.h, it renames the old
version to gt-foo.h~ so that the user could compare the old version
with the current one. The cleaning in Makefile.in is updated
appropriately.  Verbosity & renaming of old files go together (in
verbose mode, the renaming is told to the user).

Some existing functions has been slightly improved, for instance to
count the number of GTY-ed variables.

The verbosity_level variable needs to be global. It will be used in
file gengtype-state.c.

The attached patch is relative to the previous patch 1 [declprog] sent
in http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00663.html

This relative patch was obtained with
 diff -u -p -b gcc/gengtype.c gcc/gengtype.h \
     --from-file ../gengtype-gcc-01-declprog/ > \
        $HOME/tmp/gengtype_patch_2_of_N__verbosity-relto01.diff

I am also attaching the gzipped cumulated patches against trunk 163987
as file
gengtype_patch_2_of_N__verbosity-cumulatedto02-trunkrev-163987.diff.gz

######################################
gcc/ChangeLog entry [of the relative patch]
2010-09-08  Jeremie Salvucci  <jeremie.salvucci@free.fr>
	    Basile Starynkevitch  <basile@starynkevitch.net>

	* gengtype.c (verbosity_level): Added variable.
	(set_gc_used): Count variables for verbosity.
	(close_output_files): Backing up files, counting written ones
	verbosily.
	(write_types): Count emitted functions for verbosity. Added debug
	messages.
	(write_enum_defn): Count structures for verbosity. Added debug
	messages.
	(gengtype_long_options): Added "verbose".
	(print_usage): Ditto.
	(main): Verbose display of parsed files.

	* gengtype.h (verbosity_level): Added declaration.

#####################################################

Ok for trunk?

Regards.

Comments

Bernd Schmidt Sept. 8, 2010, 8:02 p.m. UTC | #1 
On 09/08/2010 07:51 PM, Basile Starynkevitch wrote:
> +    {
> +      DBGPRINTF ("set_gc_used p %p '%s' nbvars %d", (void*) p, p->name, nbvars);
>      set_gc_used_type (p->type, GC_USED, NULL);
> +      nbvars++;
> +    };

Are you using "diff -b"?  If yes - please don't.  If no, this patch
breaks formatting.  Also, no semicolon after closing brace.

Patch 1/N introduces many instances of nonstandard formatting.  Please
fix it to conform to gcc coding standards.


Bernd
Laurynas Biveinis Sept. 9, 2010, 3:02 a.m. UTC | #2 
The patch looks good to me, but please see below.

2010/9/8 Basile Starynkevitch <basile@starynkevitch.net>:
> In addition of verbose messages, we added a tiny but useful feature to
> gengtype. When it is generating a file gt-foo.h, it renames the old
> version to gt-foo.h~ so that the user could compare the old version
> with the current one. The cleaning in Makefile.in is updated
> appropriately.  Verbosity & renaming of old files go together (in
> verbose mode, the renaming is told to the user).

IMHO please don't, it's a violation of KISS. If gengtype is taking on
a job of Makefile, that is only going to result in surprises, not very
harmful ones, but still. In fact, I don't even like that gengtype
checks if output file is identical to a newly generated output file
before overwriting it and I think it should be done in Makefile with
move-if-change.

A better way IMHO would be to make gengtype output into an own new
directory "gty" or "gengtype" or whatever. Then it would be up to user
to save the old directory before the new run and as a bonus,
everything could be compared in these directories with a single
command.
Basile Starynkevitch Sept. 9, 2010, 5:22 a.m. UTC | #3 
On Wed, 08 Sep 2010 22:02:01 +0200
Bernd Schmidt <bernds@codesourcery.com> wrote:

> On 09/08/2010 07:51 PM, Basile Starynkevitch wrote:
> > +    {
> > +      DBGPRINTF ("set_gc_used p %p '%s' nbvars %d", (void*) p, p->name, nbvars);
> >      set_gc_used_type (p->type, GC_USED, NULL);
> > +      nbvars++;
> > +    };
> 
> Are you using "diff -b"?  If yes - please don't.  If no, this patch
> breaks formatting.  Also, no semicolon after closing brace.

Yes I did use diff -b. I believe my file contain tab characters inserted by Emacs. Do you suggest untabifying it before diffing?

What is the correct command to make a relative patch. I have a directory containing the previous version. I am not using quilt or git. Just plain old GNU diff.

> Patch 1/N introduces many instances of nonstandard formatting.  Please
> fix it to conform to gcc coding standards.

I did read it several times before sending, but I confess not knowing all the details, or perhaps I misunderstood some. Could you help me a little bit by showing a few non conformities?

Thanks for the comments. 

And are you suggesting that once the typos are fixed, the patch is OK? Sadly, I did not understand your reply that way. What other fixes should I work on?

Cheers.
Mike Stump Sept. 9, 2010, 6 a.m. UTC | #4 
On Sep 8, 2010, at 10:22 PM, Basile Starynkevitch wrote:
> Yes I did use diff -b. I believe my file contain tab characters inserted by Emacs. Do you suggest untabifying it before diffing?

No.  In fact, tabs are preferred.

> What is the correct command to make a relative patch.

diff -N -U3 -p is nice.

>> Patch 1/N introduces many instances of nonstandard formatting.  Please
>> fix it to conform to gcc coding standards.
> 
> I did read it several times before sending, but I confess not knowing all the details, or perhaps I misunderstood some. Could you help me a little bit by showing a few non conformities?

One example:

+static void
+print_usage (void)
+{
+    printf ("Usage: %s\n", progname);

If you use emacs, and the GNU default formatting, you can just TAB, and it will adjust the line appropriately.  If not, you should look at the rest of the compiler for examples of formatting.  If you use emacs, but have modified the default formatting, you should find a way to revert back to the defaults for editing gcc files.

+      switch (p->kind) {

Double check all white spacing around {} against the style used in gcc.  In particular, you notice that the { goes on a new line.

I glanced at the rest of one patch and didn't notice much else.

> And are you suggesting that once the typos are fixed, the patch is OK?

No.  People use the phrase, Ok with these changes: ... for patches that are very close.  For patches that are not as close, we fix up what is requested and then repost.
diff mbox

Patch

--- ../gengtype-gcc-01-declprog//gengtype.c	2010-09-08 15:02:07.000000000 +0200
+++ gcc/gengtype.c	2010-09-08 16:36:26.000000000 +0200
@@ -158,6 +158,10 @@  char *write_state_filename;
 int do_dump;
 int do_debug;
 
+/* For verbose messages to the user. */
+int verbosity_level;
+
+
 static outf_p create_file (const char *, const char *);
 
 static const char * get_file_basename (const char *);
@@ -1507,8 +1511,15 @@  static void
 set_gc_used (pair_p variables)
 {
   pair_p p;
+  int nbvars = 0;
   for (p = variables; p; p = p->next)
+    {
+      DBGPRINTF ("set_gc_used p %p '%s' nbvars %d", (void*) p, p->name, nbvars);
     set_gc_used_type (p->type, GC_USED, NULL);
+      nbvars++;
+    };
+  if (verbosity_level >= 2)
+    printf ("%s used %d GTY-ed variables\n", progname, nbvars);
 }
 
 /* File mapping routines.  For each input file, there is one output .c file
@@ -1897,26 +1908,46 @@  static void
 close_output_files (void)
 {
   outf_p of;
+  int nbwrittenfiles = 0;
 
   for (of = output_files; of; of = of->next)
     {
 
-      if (!is_file_equal(of))
+      if (!is_file_equal (of))
       {
-        FILE *newfile = fopen (of->name, "w");
+	  FILE *newfile = NULL;
+	  char *backupname = concat (of->name, "~", NULL);
+	  int renamingfailed = rename (of->name, backupname);
+	  newfile = fopen (of->name, "w");
         if (newfile == NULL)
           fatal ("opening output file %s: %s", of->name, xstrerror (errno));
         if (fwrite (of->buf, 1, of->bufused, newfile) != of->bufused)
           fatal ("writing output file %s: %s", of->name, xstrerror (errno));
         if (fclose (newfile) != 0)
           fatal ("closing output file %s: %s", of->name, xstrerror (errno));
+	  nbwrittenfiles++;
+	  if (verbosity_level >= 2 && !renamingfailed)
+	    printf ("%s wrote #%-3d %s backed-up in %s\n", 
+		    progname, nbwrittenfiles, of->name, backupname);
+	  else if (verbosity_level >= 1)
+	    printf ("%s wrote #%-3d %s\n", progname, nbwrittenfiles, of->name);
+	  free (backupname);
+	}
+      else 
+	{ 
+	  /* Output file remains unchanged! */
+	  if (verbosity_level >= 2)
+	    printf ("%s kept %s\n", progname, of->name);
       }
       free(of->buf);
       of->buf = NULL;
       of->bufused = of->buflength = 0;
     }
+  if (verbosity_level >= 1)
+    printf ("%s wrote %d files.\n", progname, nbwrittenfiles);
 }
 
+
 struct flist {
   struct flist *next;
   int started_p;
@@ -2796,6 +2827,7 @@  write_types (outf_p output_header, type_
 	     const struct write_types_data *wtd)
 {
   type_p s;
+  int nbfun = 0; /* Count the emitted functions. */
 
   oprintf (output_header, "\n/* %s*/\n", wtd->comment);
   /* We first emit the macros and the declarations. Functions' code is
@@ -2890,11 +2922,26 @@  write_types (outf_p output_header, type_
 	  {
 	    type_p ss;
 	    for (ss = s->u.s.lang_struct; ss; ss = ss->next)
+	      {
+		nbfun++;
+		DBGPRINTF ("writing func #%d lang_struct ss @ %p '%s'", 
+			   nbfun, (void*) ss, ss->u.s.tag);
 	      write_func_for_structure (s, ss, NULL, wtd);
 	  }
+	  }
 	else
+	  {
+	    nbfun++;
+	    DBGPRINTF ("writing func #%d struct s @ %p '%s'", 
+		       nbfun, (void*) s, s->u.s.tag);
 	  write_func_for_structure (s, s, NULL, wtd);
       }
+      }
+    else
+      DBGPRINTF ("ignored s @ %p  '%s' gc_used#%d", 
+		 (void*)s,  s->u.s.tag, 
+		 (int) s->gc_used);
+
   for (s = param_structs; s; s = s->next)
     if (s->gc_used == GC_POINTED_TO)
       {
@@ -2906,11 +2953,27 @@  write_types (outf_p output_header, type_
 	  {
 	    type_p ss;
 	    for (ss = stru->u.s.lang_struct; ss; ss = ss->next)
+	      {
+		nbfun++;
+		DBGPRINTF ("writing func #%d param lang_struct ss @ %p '%s'", 
+			   nbfun, (void*) ss,  ss->u.s.tag);
 	      write_func_for_structure (s, ss, param, wtd);
 	  }
+	  }
 	else
+	  {
+	    nbfun++;
+	    DBGPRINTF ("writing func #%d param struct s @ %p  stru @ %p '%s'", 
+		       nbfun, (void*) s,
+		       (void*) stru,  stru->u.s.tag);
 	  write_func_for_structure (s, stru, param, wtd);
       }
+      }
+    else
+      DBGPRINTF ("ignored s @ %p", (void*)s);
+  if (verbosity_level >= 2)
+    printf ("%s emitted %d routines for %s\n", 
+	    progname, nbfun, wtd->comment);
 }
 
 static const struct write_types_data ggc_wtd =
@@ -3104,27 +3167,44 @@  static void
 write_enum_defn (type_p structures, type_p param_structs)
 {
   type_p s;
+  int nbstruct = 0;
+  int nbparamstruct = 0;
 
   if (!header_file)
     return;
   oprintf (header_file, "\n/* Enumeration of types known.  */\n");
   oprintf (header_file, "enum gt_types_enum {\n");
+
   for (s = structures; s; s = s->next)
     if (USED_BY_TYPED_GC_P (s))
       {
+	DBGPRINTF("write_enum_defn s @ %p nbstruct %d", 
+		  (void*) s, nbstruct);
+	if (UNION_OR_STRUCT_P (s))
+	  DBGPRINTF ("write_enum_defn s %p #%d is unionorstruct tagged %s",
+		     (void*) s, nbstruct, s->u.s.tag);
 	oprintf (header_file, " gt_ggc_e_");
 	output_mangled_typename (header_file, s);
 	oprintf (header_file, ",\n");
+	nbstruct++;
       }
+
   for (s = param_structs; s; s = s->next)
     if (s->gc_used == GC_POINTED_TO)
       {
+	DBGPRINTF ("write_enum_defn s %p nbparamstruct %d",
+		   (void*) s, nbparamstruct);
 	oprintf (header_file, " gt_e_");
 	output_mangled_typename (header_file, s);
 	oprintf (header_file, ",\n");
+	nbparamstruct++;
       }
+
   oprintf (header_file, " gt_types_enum_last\n");
   oprintf (header_file, "};\n");
+  if (verbosity_level >= 2)
+    printf("%s handled %d GTY-ed structures and %d parameterized structures.\n",
+	   progname, nbstruct, nbparamstruct);
 }
 
 /* Might T contain any non-pointer elements?  */
@@ -4271,6 +4351,7 @@  dump_everything (void)
 static const struct option gengtype_long_options[] = 
 {
     { "help",      no_argument, NULL, 'h' },
+    { "verbose",   no_argument, NULL, 'v' },
     { "version",   no_argument, NULL, 'V' },
     { "dump",      no_argument, NULL, 'd' },
     { "debug",     no_argument, NULL, 'D' },
@@ -4288,6 +4369,7 @@  print_usage (void)
 {
     printf ("Usage: %s\n", progname);
     printf ("\t -h | --help  \t# Give this help.\n");
+    printf ("\t -v | --verbose \t# Increase verbosity. Can be given several times.\n");
     printf ("\t -D | --debug  \t# Give lots of debug output to debug %s itself.\n", progname);
     printf ("\t -V | --version  \t# Give version information.\n");
     printf ("\t -d | --dump  \t# Dump state for debugging.\n");
@@ -4310,7 +4392,7 @@  static void
 parse_program_options (int argc, char**argv)
 {
     int opt = -1;
-    while ((opt = getopt_long (argc, argv, "hVdP:S:I:w:r:D",
+  while ((opt = getopt_long (argc, argv, "hvVdP:S:I:w:r:D",
 			       gengtype_long_options, NULL)) >= 0)
     {
 	switch (opt)
@@ -4318,6 +4400,9 @@  parse_program_options (int argc, char**a
 	case 'h': /* --help */
 	    print_usage ();
 	    break;
+	case 'v': /* --verbose */
+	  verbosity_level++;
+	  break;
 	case 'V': /* --version */
 	    print_version ();
 	    break;
@@ -4437,6 +4522,9 @@  main (int argc, char **argv)
       }
       DBGPRINT_COUNT_TYPE ("structures after parsing", structures);
       DBGPRINT_COUNT_TYPE ("param_structs after parsing", param_structs);
+      if (verbosity_level >= 1)
+	printf ("%s parsed %d files\n", 
+		progname, (int) num_gt_files);
 
     }
   else
--- ../gengtype-gcc-01-declprog//gengtype.h	2010-09-08 13:47:38.000000000 +0200
+++ gcc/gengtype.h	2010-09-08 16:09:52.000000000 +0200
@@ -154,6 +154,9 @@  enum {
   FIRST_TOKEN_WITH_VALUE = PARAM_IS
 };
 
+/* Level for verbose messages.  Gengtype tells something to its user
+   when it is positive, and tells more if it is greater.  */
+extern int verbosity_level;
 
 /* For debugging purposes of gengtype itself!  */
 extern int do_dump;