Patchwork gengtype plugin improvement last3round -patch 3 [inputfile]

login
register
mail settings
Submitter Laurynas Biveinis
Date Nov. 5, 2010, 3:51 a.m.
Message ID <AANLkTikRXmJ-e_OLmLXc2j6XCpHExPadFtzmFHwY=7VB@mail.gmail.com>
Download mbox | patch
Permalink /patch/70232/
State New
Headers show

Comments

Laurynas Biveinis - Nov. 5, 2010, 3:51 a.m.
Basile -

We are getting there and as much as I'd like to say "OK with such and
such", I'll have to ask you to submit a new patch. Basically you did
not address my last two comments from the previous round. Please take
care to see that the new patch contains only relevant changes and
nothing else.

2010/11/4 Basile Starynkevitch <basile@starynkevitch.net>:
> I did try to take most Laurynas comment's into account. However, when he asked
>> +  if (UNION_OR_STRUCT_P (s))
>> +    fn = s->u.s.line.file;
>> +  else if (s->kind == TYPE_PARAM_STRUCT)
>> +    fn = s->u.param_struct.line.file;
>>
>> Can you submit this separately?
>
> I am a bit surprised, since this patch is related to input_file also.

Before:

const char *fn = s->u.s.line.file;

After:

 if (UNION_OR_STRUCT_P (s))
   fn = s->u.s.line.file;
 else if (s->kind == TYPE_PARAM_STRUCT)
   fn = s->u.param_struct.line.file;

Why you cannot do just the following?

const input_file *fn = s->u.s.line.file;

If this becomes illegal with your patch, then this chunk is OK. If
that fixes invalid filename information with param structs that was
there before your patch, then it should be a separate patch.



+  if (*slot)
+    {
+      /* Already known input file.  */
+      free (f);
+      return (input_file*)(*slot);
+	}
+  /* New input file.  */
+  *slot = f;
+  return f;
+    }

First unaddressed comment from the last submission - indentation of
closing braces.

+#define PREDEF_SCALAR_TYPEDEF(Nam) do{ pos.line = __LINE__; \
+	do_scalar_typedef (Nam, &pos);} while (0)
+      PREDEF_SCALAR_TYPEDEF ("CUMULATIVE_ARGS");
+      PREDEF_SCALAR_TYPEDEF ("REAL_VALUE_TYPE");
+      PREDEF_SCALAR_TYPEDEF ("FIXED_VALUE_TYPE");
+      PREDEF_SCALAR_TYPEDEF ("double_int");
+      PREDEF_SCALAR_TYPEDEF ("uint64_t");
+      PREDEF_SCALAR_TYPEDEF ("uint8");
+      PREDEF_SCALAR_TYPEDEF ("jword");
+      PREDEF_SCALAR_TYPEDEF ("JCF_u2");
+      PREDEF_SCALAR_TYPEDEF ("void");
+#undef PREDEF_SCALAR_TYPEDEF
       pos.line = __LINE__ + 1;
-      do_scalar_typedef ("CUMULATIVE_ARGS", &pos);
-      pos.line++;
-      do_scalar_typedef ("REAL_VALUE_TYPE", &pos);
-      pos.line++;
-      do_scalar_typedef ("FIXED_VALUE_TYPE", &pos);
-      pos.line++;
-      do_scalar_typedef ("double_int", &pos);
-      pos.line++;
-      do_scalar_typedef ("uint64_t", &pos);
-      pos.line++;
-      do_scalar_typedef ("uint8", &pos);
-      pos.line++;
-      do_scalar_typedef ("jword", &pos);
-      pos.line++;
-      do_scalar_typedef ("JCF_u2", &pos);
-      pos.line++;
-      do_scalar_typedef ("void", &pos);
-      pos.line++;

While this is nice, this is obviously unrelated. Please drop.

@@ -4577,10 +4613,10 @@ main (int argc, char **argv)
   if (plugin_output_filename)
     {
       size_t ix = 0;
-      /* In plugin mode, we should have read a state file, and have
-         given at least one plugin file.  */
+      /* In plugin mode, we should preferably have read a state file,
+         and should have given at least one plugin file.  */
       if (!read_state_filename)
-	fatal ("No read state given in plugin mode for %s",
+	warning ("No read state given in plugin mode for %s",
 	       plugin_output_filename);

       if (nb_plugin_files == 0 || !plugin_files)
@@ -4589,7 +4625,11 @@ main (int argc, char **argv)

       /* Parse our plugin files.  */
       for (ix = 0; ix < nb_plugin_files; ix++)
-	parse_file (plugin_files[ix]);
+	{
+	  parse_file (get_input_file_name (plugin_files[ix]));
+	  DBGPRINTF ("parsed plugin file #%d %s", (int) ix,
+		     get_input_file_name (plugin_files[ix]));
+	}

       if (hit_error)
 	return 1;

Unrelated, separate patch (although the changes here probably do not
pull their weight for a separate patch) or drop.

@@ -708,6 +709,7 @@ static type_p
 type (options_p *optsp, bool nested)
 {
   const char *s;
+  static int anonymous_count; /* To generate unique pseudo-identifiers! */
   *optsp = 0;
   switch (token ())
     {
@@ -750,7 +752,32 @@ type (options_p *optsp, bool nested)
 	if (token () == ID)
 	  s = advance ();
 	else
-	  s = xasprintf ("anonymous:%s:%d", lexer_line.file, lexer_line.line);
+	  {
+	    /* We don't want to wire in the source directory (because
+	       in plugin mode, the source directory can be unavailable
+	       since gengtype has read its state).  So if the input is
+	       from GCC source directory, we use its relative path to
+	       build an anonymous unique tag.  */
+	    const char* relp = get_file_srcdir_relative_path (lexer_line.file);
+	    anonymous_count++;
+	    if (relp)
+	      {
+		/* The input file is a GCC source file, we use a double
+		   colon after anonymous.  To be sure s is truly unique,
+		   we also use anonymous_count.  */
+		s = xasprintf ("anonymous::%s:%d::%d",
+			       relp, lexer_line.line, anonymous_count);
+	      }
+	    else
+	      {
+		/* The input file is outside of GCC source tree, we use
+		   a single colon after anonymous.  To be sure s is
+		   truly unique, we also use anonymous_count.  */
+		s = xasprintf ("anonymous:%s:%d::%d",
+			       get_input_file_name (lexer_line.file),
+			       lexer_line.line, anonymous_count);
+	      }
+	  }

 	/* Unfortunately above GTY_TOKEN check does not capture the
 	   typedef struct_type GTY case.  */
@@ -787,7 +814,19 @@ type (options_p *optsp, bool nested)
       if (token () == ID)
 	s = advance ();
       else
-	s = xasprintf ("anonymous:%s:%d", lexer_line.file, lexer_line.line);
+	{
+	  /* Again, we don't want to wire in the GCC source tree
+	     directory.  */
+	  const char* relp = get_file_srcdir_relative_path (lexer_line.file);
+	  anonymous_count++;
+	  if (relp)
+	    s = xasprintf ("anonymous::%s:%d::%d",
+			   relp, lexer_line.line, anonymous_count);
+	  else
+	    s = xasprintf ("anonymous:%s:%d::%d",
+			   get_input_file_name (lexer_line.file),
+			   lexer_line.line, anonymous_count);
+	}

       if (token () == '{')
 	consume_balanced ('{', '}');

And here you did not address my comment from the last submission that
these changes should be a separate patch.

Patch

Index: gcc/gengtype.h
===================================================================
--- gcc/gengtype.h	(revision 166319)
+++ gcc/gengtype.h	(working copy)
@@ -24,15 +24,87 @@ 
 /* Sets of accepted source languages like C, C++, Ada... are
    represented by a bitmap.  */
 typedef unsigned lang_bitmap;
+/* Variable length structure representing an input file.  A hash table
+   ensure uniqueness for a given input file name.  The only function
+   allocating input_file-s is input_file_by_name.  */
+struct input_file_st

New line before comment.

+{
+  struct outf* inpoutf;	/* Cached corresponding output file, computed
+			   in get_output_file_with_visibility.  */

Align second line of the comment.

+  char inpname[1]; /* a flexible array, ended by a null char.  */

"variable-length array"

+/* Table of all input files, and its size.  */

No comma: "...files and its..."

+input_file* input_file_by_name (const char* name);
+
+
+/* For F an input_file, return the relative path to F from $(srcdir)

One empty line.

 /* An output file, suitable for definitions, that can see declarations
-   made in INPUT_FILE and is linked into every language that uses
-   INPUT_FILE.  May return NULL in plugin mode.  */
-extern outf_p get_output_file_with_visibility (const char *input_file);
+   made in INPF and is linked into every language that uses INPF.  May
+   return NULL in plugin mode.  The INPF argument is almost const, but
+   since the result is cached in its inpoutf field it cannot be
+   declared const, because this function stores the computed output
+   file in that field to speed it up for further invocations.  */
+outf_p get_output_file_with_visibility (input_file* inpf);

Drop the last clause after "because".

@@ -1508,6 +1465,8 @@  set_gc_used (pair_p variables)
   pair_p p;
   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++;
     };

Unrelated: drop or separate patch.

-/* For F a filename, return the relative path to F from $(srcdir) if the
+/* For INPF a filename, return the relative path to F from $(srcdir) if the
    latter is a prefix in F, NULL otherwise.  */

-static const char *
-get_file_srcdir_relative_path (const char *f)

Replace the remaining occurences of F to INPF.

+  const char* f = get_input_file_name (inpf);

const char *f =

@@ -1936,11 +1900,12 @@  close_output_files (void)
 		    progname, nbwrittenfiles, of->name, backupname);
 	  else if (verbosity_level >= 1)
 	    printf ("%s write #%-3d %s\n", progname, nbwrittenfiles, of->name);
-	  free (backupname);
+	  if (backupname)
+	    free (backupname);
 	}
       else
 	{
-	  /* output file remains unchanged. */
+	  /* Output file remains unchanged. */
 	  if (verbosity_level >= 2)
 	    printf ("%s keep %s\n", progname, of->name);
 	}

Separate patches. (I'd just drop the comment change).

+input_file_by_name (const char* name)
+{
+  PTR* slot;
+  input_file* f = NULL;

const char *name
PTR *slot
input_file *f