Re: encapsulating code properties

[Joel E. Denny]

On Tue, 21 Nov 2006, Joel E. Denny wrote:

> On Mon, 13 Nov 2006, Joel E. Denny wrote:
> 
> > On Sat, 11 Nov 2006, Paul Eggert wrote:
> > 
> > > The patch changes Bison to pass too many copies of structures around,
> > > when we should be passing references to structures.
> > 
> > > If we turn every '->' into a foo_bar_baz_boof_get, our code will get
> > > harder to read, with little real benefit.  I know why you encapsulated
> > > the members inside getters; but to my mind the cost exceeds the
> > > benefit, simply in terms of reading the code.
> > 
> > > The Doxygenated comments are hard to read and often don't offer much.
> > 
> > I've tried to adjust the previous patch to address these concerns.
> 
> Below is an updated version of the patch with a few corrections to the 
> scan-code.h comments.  Ok to commit?

This patch has been sitting for a while, and I'd like to move forward with 
it.  To make it easier to understand, I'm going to break it up into 
smaller steps and more carefully document each step in the ChangeLog.

The first step appears below.  It implements the code_props interface but 
does not update any other module to use it.  I believe all previous qualms 
about the larger patch appeared in this step, and I believe I've addressed 
them all.

If I hear no objections within a few days, I'll commit this step and move 
on to the next one.

Index: ChangeLog
===================================================================
RCS file: /sources/bison/bison/ChangeLog,v
retrieving revision 1.1643
diff -p -u -r1.1643 ChangeLog
--- ChangeLog   2 Jan 2007 05:13:27 -0000       1.1643
+++ ChangeLog   2 Jan 2007 21:48:06 -0000
@@ -1,3 +1,50 @@
+2007-01-02  Joel E. Denny  <address@hidden>
+
+       Encapsulate code properties and related functionality for the various
+       destructors, printers, and actions into a code_props structure and
+       interface.  This patch merely implements code_props in scan-code.h and
+       scan-code.l.  Future patches will rewrite other modules to use it.
+       Discussed starting at
+       <http://lists.gnu.org/archive/html/bison-patches/2006-11/msg00020.html>.
+       * src/location.h (EMPTY_LOCATION_INIT): Define so that it's easier to
+       consistently initialize const structs that have an empty location
+       field.
+       * src/location.c (empty_location): Initialize with EMPTY_LOCATION_INIT
+       to ensure consistency.
+       * src/scan-code.h (code_props): New structure.
+       (code_props_none_init, CODE_PROPS_NONE_INIT, code_props_none): New
+       function, macro, and const global variable for initializing a
+       code_props with no code.
+       (code_props_plain_init, code_props_symbol_action_init,
+       code_props_rule_action_init, code_props_translate_code): The rest of
+       the new code_props functional interface.  Among other things, the init
+       functions set the code_props kind field so that
+       code_props_translate_code will know whether to behave like
+       translate_symbol_action, translate_rule_action, or translate_code.
+       These old translate functions must remain until all other modules are
+       updated to use the new code_props interface.
+       (code_scanner_last_string_free): New function similar to
+       gram_scanner_last_string_free.
+       (code_scanner_free): Add documentation.
+       * src/scan-code.l: Implement the new interface.
+       (code_lex): Make it static, add a code_props* argument, and remove the
+       rule argument.
+       (last_string): New static global similar to the one in scan-gram.l.
+       (SC_RULE_ACTION): Update to use the code_props* argument to code_lex
+       instead of rule.
+       (SC_SYMBOL_ACTION): For $$, set the is_value_used member of the
+       code_props since Bison may one day use this information for destructors
+       and printers.
+       (<*><<EOF>>): Use STRING_FINISH so that last_string is set.
+       (handle_action_dollar): Use symbol_list_n_get and set used flag
+       directly since symbol_list_n_used_set is removed.
+       (translate_action): Add a code_props* argument and remove the rule,
+       action, and location arguments.  Pass the code_props* on to code_lex.
+       (translate_rule_action, translate_symbol_action, translate_code):
+       Rewrite as wrappers around the new code_props interface.
+       * src/symlist.h, src/symlist.c (symbol_list_n_used_set): Remove since
+       it would eventually need to break the encapsulation of code_props.
+
 2007-01-01  Joel E. Denny  <address@hidden>
 
        * etc/.cvsignore: New.
Index: src/location.c
===================================================================
RCS file: /sources/bison/bison/src/location.c,v
retrieving revision 1.10
diff -p -u -r1.10 location.c
--- src/location.c      12 Nov 2006 07:39:37 -0000      1.10
+++ src/location.c      2 Jan 2007 21:48:06 -0000
@@ -1,5 +1,5 @@
 /* Locations for Bison
-   Copyright (C) 2002, 2005, 2006 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2005, 2006, 2007 Free Software Foundation, Inc.
 
    This file is part of Bison, the GNU Compiler Compiler.
 
@@ -27,7 +27,7 @@
 #include "complain.h"
 #include "location.h"
 
-location const empty_location;
+location const empty_location = EMPTY_LOCATION_INIT;
 
 /* If BUF is null, add BUFSIZE (which in this case must be less than
    INT_MAX) to COLUMN; otherwise, add mbsnwidth (BUF, BUFSIZE, 0) to
Index: src/location.h
===================================================================
RCS file: /sources/bison/bison/src/location.h,v
retrieving revision 1.20
diff -p -u -r1.20 location.h
--- src/location.h      18 Dec 2006 17:27:23 -0000      1.20
+++ src/location.h      2 Jan 2007 21:48:06 -0000
@@ -1,5 +1,5 @@
 /* Locations for Bison
-   Copyright (C) 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
 
    This file is part of Bison, the GNU Compiler Compiler.
 
@@ -71,6 +71,7 @@ typedef struct
 
 #define YYLTYPE location
 
+#define EMPTY_LOCATION_INIT {{NULL, 0, 0}, {NULL, 0, 0}}
 extern location const empty_location;
 
 /* Set *LOC and adjust scanner cursor to account for token TOKEN of
Index: src/scan-code.h
===================================================================
RCS file: /sources/bison/bison/src/scan-code.h,v
retrieving revision 1.6
diff -p -u -r1.6 scan-code.h
--- src/scan-code.h     12 Nov 2006 07:39:37 -0000      1.6
+++ src/scan-code.h     2 Jan 2007 21:48:06 -0000
@@ -1,6 +1,6 @@
-/* Bison Action Scanner
+/* Bison code properties structure and scanner.
 
-   Copyright (C) 2006 Free Software Foundation, Inc.
+   Copyright (C) 2006, 2007 Free Software Foundation, Inc.
 
    This file is part of Bison, the GNU Compiler Compiler.
 
@@ -24,13 +24,149 @@
 # define SCAN_CODE_H_
 
 # include "location.h"
-# include "symlist.h"
 
-/* Keeps track of the maximum number of semantic values to the left of
-   a handle (those referenced by $0, $-1, etc.) are required by the
-   semantic actions of this grammar. */
+struct symbol_list;
+
+/**
+ * Keeps track of the maximum number of semantic values to the left of a handle
+ * (those referenced by $0, $-1, etc.) that are required by the semantic
+ * actions of this grammar.
+ */
 extern int max_left_semantic_context;
 
+/**
+ * A code passage captured from the grammar file and possibly translated,
+ * and/or properties associated with such a code passage.  Don't break
+ * encapsulation by modifying the fields directly.  Use the provided interface
+ * functions.
+ */
+typedef struct code_props {
+  /** Set by the init functions.  */
+  enum {
+    CODE_PROPS_NONE, CODE_PROPS_PLAIN,
+    CODE_PROPS_SYMBOL_ACTION, CODE_PROPS_RULE_ACTION
+  } kind;
+
+  /** \c NULL iff \c code_props::kind is \c CODE_PROPS_NONE.  */
+  char const *code;
+  /** Undefined iff \c code_props::code is \c NULL.  */
+  location location;
+
+  /**
+   * \c false iff either:
+   *   - \c code_props_translate_code has never previously been invoked for
+   *     the \c code_props that would contain the code passage associated
+   *     with \c self.  (That \c code_props is not the same as this one if this
+   *     one is for a RHS \c symbol_list node.  Instead, it's the \c code_props
+   *     for the LHS symbol of the same rule.)
+   *   - \c code_props_translate_code has been invoked for that \c code_props,
+   *     but the symbol value associated with this \c code_props was not
+   *     referenced in the code passage.
+   */
+  bool is_value_used;
+
+  /** \c NULL iff \c code_props::kind is not \c CODE_PROPS_RULE_ACTION.  */
+  struct symbol_list *rule;
+} code_props;
+
+/**
+ * \pre
+ *   - <tt>self != NULL</tt>.
+ * \post
+ *   - \c self has been overwritten to contain no code.
+ */
+void code_props_none_init (code_props *self);
+
+/** Equivalent to \c code_props_none_init.  */
+#define CODE_PROPS_NONE_INIT \
+  {CODE_PROPS_NONE, NULL, EMPTY_LOCATION_INIT, false, NULL}
+
+/** Initialized by \c CODE_PROPS_NONE_INIT with no further modification.  */
+extern code_props const code_props_none;
+
+/**
+ * \pre
+ *   - <tt>self != NULL</tt>.
+ *   - <tt>code != NULL</tt>.
+ *   - \c code is an untranslated code passage containing no Bison escapes.
+ *   - \c code was extracted from the grammar file at \c code_loc.
+ * \post
+ *   - \c self has been overwritten to represent the specified plain code
+ *     passage.
+ *   - \c self will become invalid if the caller frees \c code before invoking
+ *     \c code_props_translate_code on \c self.
+ */
+void code_props_plain_init (code_props *self, char const *code,
+                            location code_loc);
+
+/**
+ * \pre
+ *   - <tt>self != NULL</tt>.
+ *   - <tt>code != NULL</tt>.
+ *   - \c code is an untranslated code passage.  The only Bison escapes it
+ *     might contain are $$ and address@hidden, referring to a single symbol.
+ *   - \c code was extracted from the grammar file at \c code_loc.
+ * \post
+ *   - \c self has been overwritten to represent the specified symbol action.
+ *   - \c self will become invalid if the caller frees \c code before invoking
+ *     \c code_props_translate_code on \c self.
+ */
+void code_props_symbol_action_init (code_props *self, char const *code,
+                                    location code_loc);
+
+/**
+ * \pre
+ *   - <tt>self != NULL</tt>.
+ *   - <tt>code != NULL</tt>.
+ *   - <tt>rule != NULL</tt>.
+ *   - \c code is the untranslated action of the rule for which \c rule is the
+ *     LHS node.  Thus, \c code possibly contains Bison escapes such as $$, $1,
+ *     $2, etc referring to the values of the rule.
+ *   - \c code was extracted from the grammar file at \c code_loc.
+ * \post
+ *   - \c self has been overwritten to represent the specified rule action.
+ *   - \c self does not claim responsibility for the memory of \c rule.
+ *   - \c self will become invalid if:
+ *     - The caller frees \c code before invoking \c code_props_translate_code
+ *       on \c self.
+ *     - The caller frees \c rule.
+ */
+void code_props_rule_action_init (code_props *self, char const *code,
+                                  location code_loc, struct symbol_list *rule);
+
+/**
+ * \pre
+ *   - If there's a code passage contained in \c self and it contains Bison
+ *     escapes, all grammar declarations have already been parsed as they may
+ *     affect warnings and complaints issued here.
+ * \post
+ *   - All M4-special symbols and Bison escapes have been translated in
+ *     \c self->code.
+ *   - <tt>self->code != self->address@hidden</tt> unless
+ *     <tt>self->address@hidden = NULL</tt>.
+ */
+void code_props_translate_code (code_props *self);
+
+/**
+ * \pre
+ *   - None.
+ * \post
+ *   - The dynamic memory allocated by the previous invocation of
+ *     \c code_props_translate_code (if any) was freed.  The \c code_props
+ *     instance for which \c code_props_translate_code was invoked is now
+ *     invalid.
+ */
+void code_scanner_last_string_free (void);
+
+/**
+ * \pre
+ *   - None.
+ * \post
+ *   - All dynamic memory allocated during any previous invocations of
+ *     \c code_props_translate_code, \c translate_rule_action,
+ *     \c translate_symbol_action, and \c translate_code has been freed.  All
+ *     \c code_props instances may now be invalid.
+ */
 void code_scanner_free (void);
 
 /* The action of the rule R contains $$, $1 etc. referring to the values
Index: src/scan-code.l
===================================================================
RCS file: /sources/bison/bison/src/scan-code.l,v
retrieving revision 1.17
diff -p -u -r1.17 scan-code.l
--- src/scan-code.l     12 Nov 2006 07:39:37 -0000      1.17
+++ src/scan-code.l     2 Jan 2007 21:48:06 -0000
@@ -1,6 +1,6 @@
 /* Bison Action Scanner                             -*- C -*-
 
-   Copyright (C) 2006 Free Software Foundation, Inc.
+   Copyright (C) 2006, 2007 Free Software Foundation, Inc.
 
    This file is part of Bison, the GNU Compiler Compiler.
 
@@ -39,10 +39,11 @@
 #include <quote.h>
 
 #include "scan-code.h"
+#include "symlist.h"
 
 /* The current calling start condition: SC_RULE_ACTION or
    SC_SYMBOL_ACTION. */
-# define YY_DECL char *code_lex (int sc_context, symbol_list *rule)
+# define YY_DECL static char *code_lex (code_props *self, int sc_context)
 YY_DECL;
 
 #define YY_USER_ACTION  location_compute (loc, &loc->end, yytext, yyleng);
@@ -53,6 +54,9 @@ static void handle_action_at (symbol_lis
 static location the_location;
 static location *loc = &the_location;
 
+/* A string representing the most recent translation.  */
+static char *last_string;
+
 /* True if an untyped $$ or $n was seen.  */
 static bool untyped_var_seen;
 %}
@@ -151,8 +155,12 @@ splice      (\\[ \f\t\v]*\n)*
 
 <SC_RULE_ACTION>
 {
-  "$"("<"{tag}">")?(-?[0-9]+|"$")   handle_action_dollar (rule, yytext, *loc);
-  "@"(-?[0-9]+|"$")                handle_action_at (rule, yytext, *loc);
+  "$"("<"{tag}">")?(-?[0-9]+|"$")  {
+    handle_action_dollar (self->rule, yytext, *loc);
+  }
+  "@"(-?[0-9]+|"$") {
+    handle_action_at (self->rule, yytext, *loc);
+  }
 
   "$"  {
     warn_at (*loc, _("stray `$'"));
@@ -190,7 +198,10 @@ splice      (\\[ \f\t\v]*\n)*
 
 <SC_SYMBOL_ACTION>
 {
-  "$$"   obstack_sgrow (&obstack_for_string, "]b4_dollar_dollar[");
+  "$$" {
+    obstack_sgrow (&obstack_for_string, "]b4_dollar_dollar[");
+    self->is_value_used = true;
+  }
   "@$"   obstack_sgrow (&obstack_for_string, "]b4_at_dollar[");
 }
 
@@ -215,8 +226,8 @@ splice       (\\[ \f\t\v]*\n)*
 
  /* End of processing. */
 <*><<EOF>>      {
-                   obstack_1grow (&obstack_for_string, '\0');
-                  return obstack_finish (&obstack_for_string);
+                   STRING_FINISH;
+                   return last_string;
                  }
 
 %%
@@ -238,7 +249,7 @@ int max_left_semantic_context = 0;
 static void
 handle_action_dollar (symbol_list *rule, char *text, location dollar_loc)
 {
-  const char *type_name = NULL;
+  char const *type_name = NULL;
   char *cp = text + 1;
   symbol_list *effective_rule;
   int effective_rule_length;
@@ -321,7 +332,8 @@ handle_action_dollar (symbol_list *rule,
          obstack_fgrow3 (&obstack_for_string,
                          "]b4_rhs_value(%d, %d, [%s])[",
                          effective_rule_length, n, type_name);
-         symbol_list_n_used_set (effective_rule, n, true);
+          if (n > 0)
+            symbol_list_n_get (effective_rule, n)->used = true;
        }
       else
        complain_at (dollar_loc, _("integer out of range: %s"), quote (text));
@@ -368,12 +380,11 @@ handle_action_at (symbol_list *rule, cha
 | Initialize the scanner.  |
 `-------------------------*/
 
-/* Translate the dollars and ats in \a a, whose location is \a l.  The
-   translation is for \a rule, in the context \a sc_context
+/* Translate the dollars and ats in \a self, in the context \a sc_context
    (SC_RULE_ACTION, SC_SYMBOL_ACTION, INITIAL).  */
 
 static char const *
-translate_action (int sc_context, symbol_list *rule, char const *a, location l)
+translate_action (code_props *self, int sc_context)
 {
   char *res;
   static bool initialized = false;
@@ -384,36 +395,82 @@ translate_action (int sc_context, symbol
       initialized = true;
     }
 
-  loc->start = loc->end = l.start;
-  yy_switch_to_buffer (yy_scan_string (a));
-  res = code_lex (sc_context, rule);
+  loc->start = loc->end = self->location.start;
+  yy_switch_to_buffer (yy_scan_string (self->code));
+  res = code_lex (self, sc_context);
   yy_delete_buffer (YY_CURRENT_BUFFER);
 
   return res;
 }
 
-char const *
-translate_rule_action (symbol_list *rule)
+/*------------------------------------------------------------------------.
+| Implementation of the public interface as documented in "scan-code.h".  |
+`------------------------------------------------------------------------*/
+
+void
+code_props_none_init (code_props *self)
 {
-  return translate_action (SC_RULE_ACTION, rule, rule->action,
-                          rule->action_location);
+  *self = code_props_none;
 }
 
-char const *
-translate_symbol_action (char const *a, location l)
+code_props const code_props_none = CODE_PROPS_NONE_INIT;
+
+void
+code_props_plain_init (code_props *self, char const *code, location code_loc)
 {
-  return translate_action (SC_SYMBOL_ACTION, NULL, a, l);
+  self->kind = CODE_PROPS_PLAIN;
+  self->code = code;
+  self->location = code_loc;
+  self->is_value_used = false;
+  self->rule = NULL;
 }
 
-char const *
-translate_code (char const *a, location l)
+void
+code_props_symbol_action_init (code_props *self, char const *code,
+                               location code_loc)
 {
-  return translate_action (INITIAL, NULL, a, l);
+  self->kind = CODE_PROPS_SYMBOL_ACTION;
+  self->code = code;
+  self->location = code_loc;
+  self->is_value_used = false;
+  self->rule = NULL;
 }
 
-/*-----------------------------------------------.
-| Free all the memory allocated to the scanner.  |
-`-----------------------------------------------*/
+void
+code_props_rule_action_init (code_props *self, char const *code,
+                             location code_loc, symbol_list *rule)
+{
+  self->kind = CODE_PROPS_RULE_ACTION;
+  self->code = code;
+  self->location = code_loc;
+  self->is_value_used = false;
+  self->rule = rule;
+}
+
+void
+code_props_translate_code (code_props *self)
+{
+  switch (self->kind)
+    {
+      case CODE_PROPS_NONE:
+        break;
+      case CODE_PROPS_PLAIN:
+        self->code = translate_action (self, INITIAL);
+        break;
+      case CODE_PROPS_SYMBOL_ACTION:
+        self->code = translate_action (self, SC_SYMBOL_ACTION);
+        break;
+      case CODE_PROPS_RULE_ACTION:
+        self->code = translate_action (self, SC_RULE_ACTION);
+        break;
+    }
+}
+
+void
+code_scanner_last_string_free (void)
+{
+  STRING_FREE;
+}
 
 void
 code_scanner_free (void)
@@ -422,3 +479,30 @@ code_scanner_free (void)
   /* Reclaim Flex's buffers.  */
   yylex_destroy ();
 }
+
+char const *
+translate_rule_action (symbol_list *rule)
+{
+  code_props cp;
+  code_props_rule_action_init (&cp, rule->action, rule->action_location, rule);
+  code_props_translate_code (&cp);
+  return cp.code;
+}
+
+char const *
+translate_symbol_action (char const *a, location l)
+{
+  code_props cp;
+  code_props_symbol_action_init (&cp, a, l);
+  code_props_translate_code (&cp);
+  return cp.code;
+}
+
+char const *
+translate_code (char const *a, location l)
+{
+  code_props cp;
+  code_props_plain_init (&cp, a, l);
+  code_props_translate_code (&cp);
+  return cp.code;
+}
Index: src/symlist.c
===================================================================
RCS file: /sources/bison/bison/src/symlist.c,v
retrieving revision 1.26
diff -p -u -r1.26 symlist.c
--- src/symlist.c       21 Nov 2006 00:43:26 -0000      1.26
+++ src/symlist.c       2 Jan 2007 21:48:07 -0000
@@ -1,6 +1,6 @@
 /* Lists of symbols for Bison
 
-   Copyright (C) 2002, 2005, 2006 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2005, 2006, 2007 Free Software Foundation, Inc.
 
    This file is part of Bison, the GNU Compiler Compiler.
 
@@ -207,18 +207,6 @@ symbol_list_n_type_name_get (symbol_list
 }
 
 
-/*--------------------------------------.
-| The item N in symbol list L is USED.  |
-`--------------------------------------*/
-
-void
-symbol_list_n_used_set (symbol_list *l, int n, bool used)
-{
-  l = symbol_list_n_get (l, n);
-  if (l)
-    l->used = used;
-}
-
 void
 symbol_list_destructor_set (symbol_list *node, const char *destructor,
                             location loc)
Index: src/symlist.h
===================================================================
RCS file: /sources/bison/bison/src/symlist.h,v
retrieving revision 1.23
diff -p -u -r1.23 symlist.h
--- src/symlist.h       21 Nov 2006 00:43:26 -0000      1.23
+++ src/symlist.h       2 Jan 2007 21:48:07 -0000
@@ -1,6 +1,6 @@
 /* Lists of symbols for Bison
 
-   Copyright (C) 2002, 2005, 2006 Free Software Foundation, Inc.
+   Copyright (C) 2002, 2005, 2006, 2007 Free Software Foundation, Inc.
 
    This file is part of Bison, the GNU Compiler Compiler.
 
@@ -106,9 +106,6 @@ symbol_list *symbol_list_n_get (symbol_l
    symbol N in rule RULE.  */
 uniqstr symbol_list_n_type_name_get (symbol_list *l, location loc, int n);
 
-/** The item \c n in symbol list \c l is \c used.  */
-void symbol_list_n_used_set (symbol_list *l, int n, bool used);
-
 /** Set the \c \%destructor for \c node as \c destructor at \c loc.  */
 void symbol_list_destructor_set (symbol_list *node, const char *destructor,
                                  location loc);