Re: gengtype plugin improvement last4round -patch 4 [filerules]

[Laurynas Biveinis <laurynas dot biveinis at gmail dot com>]

Hello,

I know that upon last reading of this patch I didn't have the comments
below and I'm sorry for that.

+struct file_rule_st {
+  const char* frul_srcexpr;	/* Source string for regexp.  */
+  int frul_rflags;		/* Flags passed to regcomp, usually
+				 * REG_EXTENDED.  */
+  regex_t* frul_re;		/* Compiled regular expression
+				   obtained by regcomp.  */
+  const char* frul_tr_out;	/* Transform string for making the

"Transformation string"

+				 * output_name, with $1 ... $9 for
+				 * subpatterns and $0 for the whole
+				 * matched filename.  */
+  const char* frul_tr_for;	/* Tranform string for making the

"Transformation string"

+				   for_name.  */
+  frul_actionrout_t* frul_action; /* The action, if non null, is
+				   * called once the rule matches, on
+				   * the transformed out_name &
+				   * for_name.  It could change them
+				   * and/or give the output file.  */
+};

+/* The array of our rules governing file name generation.  Rules order
+   matters!  Change these rules with extreme care!  */

"Rule order matters".  Also IMHO the last sentence is redundant.

+struct file_rule_st files_rules[] = {
+  /* the c-family/ source directory is special.  */
+  { "^(([^/]*/)*)c-family/([[:alnum:]_-]*)\\.c$",
+    REG_EXTENDED, NULL_REGEX,
+    "gt-c-family-$3.h", "c-family/$3.c", NULL_FRULACT},
+
+  { "^(([^/]*/)*)c-family/([[:alnum:]_-]*)\\.h$",
+    REG_EXTENDED, NULL_REGEX,
+    "gt-c-family-$3.h", "c-family/$3.h", NULL_FRULACT},

I wonder if here we could do something like

#define DIR_PREFIX "^(([^/]*/)*)"

struct file_rule_st files_rules[] = {
...
  { DIR_PREFIX "c-family/([[:alnum:]_-]*)\\.c$",
...

  { DIR_PREFIX "c-family/([[:alnum:]_-]*)\\.h$",

etc.

+/* Special file rules action for handling *.h header files.  It gives
+   "gtype-desc.c" for common headers and compute language specific
+   header files.  */

...and corresponding output files for language-specific header files.

+static outf_p
+header_dot_h_frul (input_file* inpf, char**poutname, char**pforname)
+{
+  const char *basename = 0;
+  int lang_index = 0;
+  const char* inpname = get_input_file_name (inpf);

const char *inpname

+      /* The header is common to all front-end languages.  So
+	 output_name is "gtype-desc.c" file.  The calling function
+	 get_output_file_with_visibility will find its outf_p.  */
+      free (*poutname);
+      *poutname = CONST_CAST (char*, "gtype-desc.c");

I am not sure about memory management here, i.e. mixing of
heap-allocated and constant strings. How about we return
xstrdup("gtype-desc.c"). It is more robust if someone plays with this
logic.

+static outf_p
+source_dot_c_frul (input_file* inpf, char**poutname, char**pforname)
+{
+  char *newbasename = NULL;
+  char* newoutname = NULL;

Why not just initialize these two with their proper values:

char *newbasename = CONST_CAST (char*, get_file_basename (inpf));
char *newoutname = CONST_CAST (char*, get_file_gtfilename (inpf));

Also, char *newoutname (star attaches to the variable).

+  const char* inpname = get_input_file_name (inpf);

char *foo

+  DBGPRINTF ("inpf %p inpname %s oriiginal outname %s forname %s",
+	     (void*) inpf, inpname, *poutname, *pforname);

"original"

+static char*
+matching_file_name_substitute (const char *filnam, regmatch_t pmatch[10],
+			       const char* trs)

const char *trs

+  char* str = NULL;
+  char* rawstr = NULL;
+  const char* pt = NULL;

char *foo

+	else
+	  /* This can happen only when files_rules is buggy! */
+	  fatal ("invalid dollar in transform string %s", trs);

Please surround with curly braces and there is no need for
diagnostics, this would be a bug in gengtype, not an user error:

else
  {
     /* This can happen only when files_rules is buggy!  */
     ggc_unreachable();
 }

+  obstack_1grow (&str_obstack, (char) 0);

obstack_1grow (&str_obstack, '\0');

In get_output_file_with_visibility:

+  /* Use our files_rules machinery, by trying each rule in sequence
+     until a rule is triggered.  Rule regexpr-s are compiled only once,
+     lazily.  */

/* Try each rule in sequence in files_rules until one is triggered. */

+	    /* The regexpr has not been compiled yet.  We lazily
+	       compile it only once.  */

/* Compile the regexpr lazily.  */

+	    if (err)
+	      {
+		/* The regular expression compilation fails only when
+		   file_rules is buggy.  We give a possibly truncated
+		   error message in this impossible case, when the
+		   files_rules sequence is erroneous.  */
+		char rxerrbuf[128];
+		memset (rxerrbuf, 0, sizeof (rxerrbuf));
+		regerror (err, files_rules[rulix].frul_re,
+			  rxerrbuf, sizeof (rxerrbuf)-1);
+		fatal ("file rule #%d regexpr error %s", rulix, rxerrbuf);
+	      }

if (err)
  {
    /* The regular expression compilation fails only when file_rules
is buggy.  */
    ggc_unreachable();
  }

+  if (!output_name || !for_name)
+    {
+      /* This is impossible, and could only happen if the files_rules is
+	 incomplete or buggy.  */
+      fatal ("no output name for input file %s, files_rules is buggy!",
+	     inpfname);
     }

Likewise.


Thanks,
-- 
Laurynas