gengtype improvements for plugins. patch 3/N [input_file]

Laurynas Biveinis laurynas.biveinis@gmail.com
Fri Sep 3 08:47:00 GMT 2010


Again, I assume that this_file and system_h_file will be used
somewhere else outside gengtype.c, is that correct?

> 	(get_file_srcdir_relative_path, get_file_langdir): Changed formal
> 	to input_file* in function declarations.

"Change type of the argument to input_file*"

> 	(read_input_line): Updated comment. Need no more the horrible
> 	lang_bitmap space kludge!

Drop the "whys" :) Just "Update comment".

> 	* gengtype.h: Added new input_file type and upgraded declarations.
> 	Moved many definitions from gengtype.c & declared several of its
> 	functions.

What is an upgraded declaration?

> 	* gengtype-lex.l: Added include of errors.h

Include errors.h.

+/* Variable length structure representing an input file. An hash table

".. A hash..."

+   ensure uniqueness for a given input file name. */
+struct input_file_st
+{
+  int inpmagic;		/* Always INPUT_FILE_MAGIC.  */

Does this magic buy us anything? If you were debugging a memory
corruption, I guess Valgrind would have caught it as well (strictly
speaking, not necessarily - but quite likely). Now that you are done
with debugging, might as well remove it. :)

+#define INPUT_FILE_MAGIC 924599103 /* 0x371c433f */

Please move outside of struct, define in hex, drop the dec.

+  struct outf* inpoutf;	/* Cached corresponding output file, computed
+			   in get_output_file_with_visibility. */
+  lang_bitmap inpbitmap; /* The set of languages using this file. */
+  char inpname[1]; /* a flexible array, ended by a null char. */
+};

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

and its size.

+extern input_file** gt_files;
+extern size_t num_gt_files;

+#if ENABLE_CHECKING
+    if (inpf->inpmagic != INPUT_FILE_MAGIC)
+      fatal("corrupted input file %p -bad magic %d",
+	    (const void*) inpf, inpf->inpmagic);
+#endif

If this is to be kept (see my comments about magic above), then
refactor it out to some function and renamve input_file_name to
get_input_file_name for symmetry with get/set_lang_bitmap. Otherwise,
inline input_file_name, get_lang_bitmap, set_lang_bitmap as they
become one-liners if checking is removed.

+/* A file location, mostly for error messages.  */
 struct fileloc {
-  const char *file;
+  input_file *ifile;
   int line;
 };

.IMHO a rename not necessary (also would help to reduce the patch size).

--- ../gengtype-gcc-02/gengtype-lex.l	2010-08-29 11:04:49.000000000 +0200
+++ gcc/gengtype-lex.l	2010-08-29 11:07:30.000000000 +0200
...
+#include "errors.h"             /* for fatal */
...
-  lexer_line.file = fname;
+  lexer_line.ifile = input_file_by_name (fname);

You are not using fatal here. Either remove the include or adjust the
comment why this is needed.  Same for gengtype-parse.c.

--- ../gengtype-gcc-02/gengtype.c	2010-08-29 08:17:58.000000000 +0200
+++ gcc/gengtype.c	2010-08-29 11:25:37.000000000 +0200

+/* The plugin input files and their number; in that case only a single
+   file is produced.

In what case?

+/****************************************************************
+ * Manage input files.
+ ****************************************************************/

Follow the style used elsewhere for source file sections (IIRC a ^L
and a single line comment)

+input_file*
+input_file_by_name (const char* name)

Needs a comment.

+    /* Fresh input file. */

New input file.

-/* For F a filename, return the relative path to F from $(srcdir) if the
-   latter is a prefix in F, NULL otherwise.  */
+/* For F an input_file, 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)
+get_file_srcdir_relative_path (const input_file *inpf)

/* For INPF, return the relative path to INPF from $(srcdir) if the
latter ... */

Similar in further comments: no need to describe what inpf is, comment
should also refer to INPF not F.

>        output_name = "gtype-desc.c";

If you made definitions for system.h and gengtype.c, might as well
provide one for gtype-desc.c too.

I like the patch, but it IMHO it needs further work.

-- 
Laurynas



More information about the Gcc-patches mailing list