Re: [PATCH] Use opaque struct rather than char * in regex.h

[Paul Eggert]

On 04/03/2012 09:34 PM, Jim Meyering wrote:
> The alternative (your patch or similar) would move the definition
> of re_dfa_t from its private header to a public one.

We needn't expose the typedef, just the struct tag for an incomplete
type; this doesn't reveal anything other than the struct tag, and the
tag begins with re_ so it is in the POSIX reserved space and POSIX
would allow this change.

Also, changing the type makes the code more honest.  The code
never uses the value according to its declared type 'unsigned char *',
so that type is bogus.  I expect it's there only for portability to
pre-C89 compilers, which is no longer an issue with either gnulib or with
glibc.  So whenever we merge gnulib's changes into glibc, we might
as well merge this in too.

However, the patch kept a number of unnecessary casts.  I installed
the following instead.  And thanks for the bug report!

regex: remove unnecessary type punning
Problem reported by Vladimir Serbinenko in
<http://lists.gnu.org/archive/html/bug-gnulib/2012-04/msg00006.html>.
* lib/regex.h (struct re_pattern_buffer): Change the type of
__REPB_PREFIX(buffer) from unsigned char * to struct re_dfa_t *.
Fix comment to match code.
* lib/regcomp.c (re_compile_fastmap, re_compile_fastmap_iter, regfree)
(re_compile_internal, free_workarea_compile, analyze, lower_subexp)
(parse, parse_reg_exp, parse_branch, parse_expression, parse_sub_exp):
* lib/regexec.c (regexec, re_search_stub, re_search_internal)
(set_regs):
Omit no-longer-necessary casts.
diff --git a/lib/regcomp.c b/lib/regcomp.c
index 311f2d5..b51a9a6 100644
--- a/lib/regcomp.c
+++ b/lib/regcomp.c
@@ -272,7 +272,7 @@ int
 re_compile_fastmap (bufp)
     struct re_pattern_buffer *bufp;
 {
-  re_dfa_t *dfa = (re_dfa_t *) bufp->buffer;
+  re_dfa_t *dfa = bufp->buffer;
   char *fastmap = bufp->fastmap;

   memset (fastmap, '\0', sizeof (char) * SBC_MAX);
@@ -306,7 +306,7 @@ static void
 re_compile_fastmap_iter (regex_t *bufp, const re_dfastate_t *init_state,
                         char *fastmap)
 {
-  re_dfa_t *dfa = (re_dfa_t *) bufp->buffer;
+  re_dfa_t *dfa = bufp->buffer;
   Idx node_cnt;
   bool icase = (dfa->mb_cur_max == 1 && (bufp->syntax & RE_ICASE));
   for (node_cnt = 0; node_cnt < init_state->nodes.nelem; ++node_cnt)
@@ -660,7 +660,7 @@ void
 regfree (preg)
     regex_t *preg;
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   if (BE (dfa != NULL, 1))
     free_dfa_content (dfa);
   preg->buffer = NULL;
@@ -767,7 +767,7 @@ re_compile_internal (regex_t *preg, const char * pattern, 
size_t length,
   preg->regs_allocated = REGS_UNALLOCATED;

   /* Initialize the dfa.  */
-  dfa = (re_dfa_t *) preg->buffer;
+  dfa = preg->buffer;
   if (BE (preg->allocated < sizeof (re_dfa_t), 0))
     {
       /* If zero allocated, but buffer is non-null, try to realloc
@@ -778,7 +778,7 @@ re_compile_internal (regex_t *preg, const char * pattern, 
size_t length,
       if (dfa == NULL)
        return REG_ESPACE;
       preg->allocated = sizeof (re_dfa_t);
-      preg->buffer = (unsigned char *) dfa;
+      preg->buffer = dfa;
     }
   preg->used = sizeof (re_dfa_t);

@@ -993,7 +993,7 @@ init_word_char (re_dfa_t *dfa)
 static void
 free_workarea_compile (regex_t *preg)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   bin_tree_storage_t *storage, *next;
   for (storage = dfa->str_tree_storage; storage; storage = next)
     {
@@ -1177,7 +1177,7 @@ optimize_utf8 (re_dfa_t *dfa)
 static reg_errcode_t
 analyze (regex_t *preg)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   reg_errcode_t ret;

   /* Allocate arrays.  */
@@ -1358,7 +1358,7 @@ lower_subexps (void *extra, bin_tree_t *node)
 static bin_tree_t *
 lower_subexp (reg_errcode_t *err, regex_t *preg, bin_tree_t *node)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   bin_tree_t *body = node->left;
   bin_tree_t *op, *cls, *tree1, *tree;

@@ -2139,7 +2139,7 @@ static bin_tree_t *
 parse (re_string_t *regexp, regex_t *preg, reg_syntax_t syntax,
        reg_errcode_t *err)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   bin_tree_t *tree, *eor, *root;
   re_token_t current_token;
   dfa->syntax = syntax;
@@ -2173,7 +2173,7 @@ static bin_tree_t *
 parse_reg_exp (re_string_t *regexp, regex_t *preg, re_token_t *token,
               reg_syntax_t syntax, Idx nest, reg_errcode_t *err)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   bin_tree_t *tree, *branch = NULL;
   tree = parse_branch (regexp, preg, token, syntax, nest, err);
   if (BE (*err != REG_NOERROR && tree == NULL, 0))
@@ -2215,7 +2215,7 @@ parse_branch (re_string_t *regexp, regex_t *preg, 
re_token_t *token,
              reg_syntax_t syntax, Idx nest, reg_errcode_t *err)
 {
   bin_tree_t *tree, *expr;
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   tree = parse_expression (regexp, preg, token, syntax, nest, err);
   if (BE (*err != REG_NOERROR && tree == NULL, 0))
     return NULL;
@@ -2259,7 +2259,7 @@ static bin_tree_t *
 parse_expression (re_string_t *regexp, regex_t *preg, re_token_t *token,
                  reg_syntax_t syntax, Idx nest, reg_errcode_t *err)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   bin_tree_t *tree;
   switch (token->type)
     {
@@ -2475,7 +2475,7 @@ static bin_tree_t *
 parse_sub_exp (re_string_t *regexp, regex_t *preg, re_token_t *token,
               reg_syntax_t syntax, Idx nest, reg_errcode_t *err)
 {
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
   bin_tree_t *tree;
   size_t cur_nsub;
   cur_nsub = preg->re_nsub++;
diff --git a/lib/regex.h b/lib/regex.h
index c1cd613..ab8bb8c 100644
--- a/lib/regex.h
+++ b/lib/regex.h
@@ -421,10 +421,9 @@ typedef enum

 struct re_pattern_buffer
 {
-  /* Space that holds the compiled pattern.  It is declared as
-     'unsigned char *' because its elements are sometimes used as
-     array indexes.  */
-  unsigned char *__REPB_PREFIX(buffer);
+  /* Space that holds the compiled pattern.  The type
+     'struct re_dfa_t' is private and is not declared here.  */
+  struct re_dfa_t *__REPB_PREFIX(buffer);

   /* Number of bytes to which 'buffer' points.  */
   __re_long_size_t __REPB_PREFIX(allocated);
diff --git a/lib/regexec.c b/lib/regexec.c
index 5cbfa91..92efb44 100644
--- a/lib/regexec.c
+++ b/lib/regexec.c
@@ -228,7 +228,7 @@ regexec (preg, string, nmatch, pmatch, eflags)
   reg_errcode_t err;
   Idx start, length;
 #ifdef _LIBC
-  re_dfa_t *dfa = (re_dfa_t *) preg->buffer;
+  re_dfa_t *dfa = preg->buffer;
 #endif

   if (eflags & ~(REG_NOTBOL | REG_NOTEOL | REG_STARTEND))
@@ -421,7 +421,7 @@ re_search_stub (struct re_pattern_buffer *bufp,
   regoff_t rval;
   int eflags = 0;
 #ifdef _LIBC
-  re_dfa_t *dfa = (re_dfa_t *) bufp->buffer;
+  re_dfa_t *dfa = bufp->buffer;
 #endif
   Idx last_start = start + range;

@@ -639,7 +639,7 @@ re_search_internal (const regex_t *preg,
                    int eflags)
 {
   reg_errcode_t err;
-  const re_dfa_t *dfa = (const re_dfa_t *) preg->buffer;
+  const re_dfa_t *dfa = preg->buffer;
   Idx left_lim, right_lim;
   int incr;
   bool fl_longest_match;
@@ -1432,7 +1432,7 @@ internal_function __attribute_warn_unused_result__
 set_regs (const regex_t *preg, const re_match_context_t *mctx, size_t nmatch,
          regmatch_t *pmatch, bool fl_backtrack)
 {
-  const re_dfa_t *dfa = (const re_dfa_t *) preg->buffer;
+  const re_dfa_t *dfa = preg->buffer;
   Idx idx, cur_node;
   re_node_set eps_via_nodes;
   struct re_fail_stack_t *fs;