Thread: Improving the notation for ecpg.addons rules

Improving the notation for ecpg.addons rules

From
Tom Lane
Date:
I've gotten annoyed by the notation used for ecpg.addons rules,
in which all the tokens of the gram.y rule to be modified
are just concatenated.  This is unreadable and potentially
ambiguous:

ECPG: fetch_argsABSOLUTE_PSignedIconstopt_from_incursor_name addon

The attached draft patch changes things so that we can write

ECPG: addon fetch_args ABSOLUTE_P SignedIconst opt_from_in cursor_name

which is a whole lot closer to what actually appears in gram.y:

fetch_args:    cursor_name
            ...
            | ABSOLUTE_P SignedIconst opt_from_in cursor_name

(Note that I've also moved the rule type "addon" to the front.
This isn't strictly necessary, but it seems less mistake-prone.)

While I've not done it in the attached, perhaps it would be
an improvement to allow a colon after the target nonterminal:

ECPG: addon fetch_args: ABSOLUTE_P SignedIconst opt_from_in cursor_name

to bring it even closer to what is written in gram.y.  You could
imagine going further and writing this case as something like

ECPG: addon fetch_args | ABSOLUTE_P SignedIconst opt_from_in cursor_name

but I think that might be a step too far.  IMO it's not adding much
readability, and it seems like introducing an unnecessary dependency
on exactly how the gram.y alternatives are laid out.

BTW, the attached patch won't apply to HEAD, it's meant to apply
after the patch series being discussed at [1].  So I won't stick
this in the CF yet.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2011420.1713493114@sss.pgh.pa.us

diff --git a/src/interfaces/ecpg/preproc/README.parser b/src/interfaces/ecpg/preproc/README.parser
index d6bb872165..11b6e102b6 100644
--- a/src/interfaces/ecpg/preproc/README.parser
+++ b/src/interfaces/ecpg/preproc/README.parser
@@ -40,19 +40,19 @@ continue to use the normal Bison notations.)


 ecpg.addons contains entries that begin with a line like
-       ECPG: concattokens ruletype
+       ECPG: ruletype tokenlist
 and typically have one or more following lines that are the code
 for a grammar action.  Any line not starting with "ECPG:" is taken
 to be part of the code block for the preceding "ECPG:" line.

-"concattokens" identifies which gram.y production this entry affects.
-It is simply the target nonterminal and the tokens from the gram.y rule
-concatenated together.  For example, to modify the action for a gram.y
-rule like this:
+"tokenlist" identifies which gram.y production this entry affects.
+It is simply a list of the target nonterminal and the input tokens
+from the gram.y rule.  For example, to modify the action for a
+gram.y rule like this:
       target: tokenA tokenB tokenC {...}
-"concattokens" would be "targettokenAtokenBtokenC".  If we want to
+"tokenlist" would be "target tokenA tokenB tokenC".  If we want to
 modify a non-first alternative for a nonterminal, we still write the
-nonterminal.  For example, "concattokens" should be "targettokenDtokenE"
+nonterminal.  For example, "tokenlist" should be "target tokenD tokenE"
 to affect the second alternative in:
       target: tokenA tokenB tokenC {...}
               | tokenD tokenE {...}
@@ -72,7 +72,7 @@ c) "rule" - the automatic action is emitted, but then the entry's
     code block is added verbatim afterwards.  This typically is
     used to add new alternatives to a nonterminal of the core grammar.
     For example, given the entry:
-      ECPG: targettokenAtokenBtokenC rule
+      ECPG: rule target tokenA tokenB tokenC
           | tokenD tokenE { custom_action; }
     what will be emitted is
       target: tokenA tokenB tokenC { automatic_action; }
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 05de4ff1f1..71f7ad26ad 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -1,5 +1,5 @@
 /* src/interfaces/ecpg/preproc/ecpg.addons */
-ECPG: stmtClosePortalStmt block
+ECPG: block stmt ClosePortalStmt
     {
         if (INFORMIX_MODE)
         {
@@ -16,23 +16,23 @@ ECPG: stmtClosePortalStmt block

         output_statement(@1, 0, ECPGst_normal);
     }
-ECPG: stmtDeallocateStmt block
+ECPG: block stmt DeallocateStmt
     {
         output_deallocate_prepare_statement(@1);
     }
-ECPG: stmtDeclareCursorStmt block
+ECPG: block stmt DeclareCursorStmt
     {
         output_simple_statement(@1, (strncmp(@1, "ECPGset_var", strlen("ECPGset_var")) == 0) ? 4 : 0);
     }
-ECPG: stmtDiscardStmt block
-ECPG: stmtFetchStmt block
+ECPG: block stmt DiscardStmt
+ECPG: block stmt FetchStmt
     { output_statement(@1, 1, ECPGst_normal); }
-ECPG: stmtDeleteStmt block
-ECPG: stmtInsertStmt block
-ECPG: stmtSelectStmt block
-ECPG: stmtUpdateStmt block
+ECPG: block stmt DeleteStmt
+ECPG: block stmt InsertStmt
+ECPG: block stmt SelectStmt
+ECPG: block stmt UpdateStmt
     { output_statement(@1, 1, ECPGst_prepnormal); }
-ECPG: stmtExecuteStmt block
+ECPG: block stmt ExecuteStmt
     {
         check_declared_list($1.name);
         if ($1.type == NULL || strlen($1.type) == 0)
@@ -57,7 +57,7 @@ ECPG: stmtExecuteStmt block
             output_statement(cat_str(3, "execute", "$0", $1.type), 0, ECPGst_exec_with_exprlist);
         }
     }
-ECPG: stmtPrepareStmt block
+ECPG: block stmt PrepareStmt
     {
         check_declared_list($1.name);
         if ($1.type == NULL)
@@ -87,17 +87,17 @@ ECPG: stmtPrepareStmt block
             output_statement(cat_str(5, "prepare", "$0", $1.type, "as", $1.stmt), 0, ECPGst_prepare);
         }
     }
-ECPG: stmtTransactionStmt block
+ECPG: block stmt TransactionStmt
     {
         fprintf(base_yyout, "{ ECPGtrans(__LINE__, %s, \"%s\");", connection ? connection : "NULL", @1);
         whenever_action(2);
     }
-ECPG: toplevel_stmtTransactionStmtLegacy block
+ECPG: block toplevel_stmt TransactionStmtLegacy
     {
         fprintf(base_yyout, "{ ECPGtrans(__LINE__, %s, \"%s\");", connection ? connection : "NULL", @1);
         whenever_action(2);
     }
-ECPG: stmtViewStmt rule
+ECPG: rule stmt ViewStmt
     | ECPGAllocateDescr
     {
         fprintf(base_yyout, "ECPGallocate_desc(__LINE__, %s);", @1);
@@ -233,42 +233,42 @@ ECPG: stmtViewStmt rule

         output_simple_statement(@1, 0);
     }
-ECPG: where_or_current_clauseWHERECURRENT_POFcursor_name block
+ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
     {
         const char *cursor_marker = @4[0] == ':' ? "$0" : @4;

         @$ = cat_str(2, "where current of", cursor_marker);
     }
-ECPG:
CopyStmtCOPYopt_binaryqualified_nameopt_column_listcopy_fromopt_programcopy_file_namecopy_delimiteropt_withcopy_optionswhere_clause
addon
+ECPG: addon CopyStmt COPY opt_binary qualified_name opt_column_list copy_from opt_program copy_file_name
copy_delimiteropt_with copy_options where_clause 
         if (strcmp(@6, "from") == 0 &&
             (strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
             mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
-ECPG: var_valueNumericOnly addon
+ECPG: addon var_value NumericOnly
         if (@1[0] == '$')
             @$ = "$0";
-ECPG: fetch_argscursor_name addon
+ECPG: addon fetch_args cursor_name
         struct cursor *ptr = add_additional_variables(@1, false);

         update_connection(ptr->connection);
         if (@1[0] == ':')
             @$ = "$0";
-ECPG: fetch_argsfrom_incursor_name addon
+ECPG: addon fetch_args from_in cursor_name
         struct cursor *ptr = add_additional_variables(@2, false);

         update_connection(ptr->connection);
         if (@2[0] == ':')
             @$ = cat2_str(@1, "$0");
-ECPG: fetch_argsNEXTopt_from_incursor_name addon
-ECPG: fetch_argsPRIORopt_from_incursor_name addon
-ECPG: fetch_argsFIRST_Popt_from_incursor_name addon
-ECPG: fetch_argsLAST_Popt_from_incursor_name addon
-ECPG: fetch_argsALLopt_from_incursor_name addon
+ECPG: addon fetch_args NEXT opt_from_in cursor_name
+ECPG: addon fetch_args PRIOR opt_from_in cursor_name
+ECPG: addon fetch_args FIRST_P opt_from_in cursor_name
+ECPG: addon fetch_args LAST_P opt_from_in cursor_name
+ECPG: addon fetch_args ALL opt_from_in cursor_name
         struct cursor *ptr = add_additional_variables(@3, false);

         update_connection(ptr->connection);
         if (@3[0] == ':')
             @$ = cat_str(3, @1, @2, "$0");
-ECPG: fetch_argsSignedIconstopt_from_incursor_name addon
+ECPG: addon fetch_args SignedIconst opt_from_in cursor_name
         struct cursor *ptr = add_additional_variables(@3, false);
         bool    replace = false;

@@ -285,17 +285,17 @@ ECPG: fetch_argsSignedIconstopt_from_incursor_name addon
         }
         if (replace)
             @$ = cat_str(3, @1, @2, @3);
-ECPG: fetch_argsFORWARDALLopt_from_incursor_name addon
-ECPG: fetch_argsBACKWARDALLopt_from_incursor_name addon
+ECPG: addon fetch_args FORWARD ALL opt_from_in cursor_name
+ECPG: addon fetch_args BACKWARD ALL opt_from_in cursor_name
         struct cursor *ptr = add_additional_variables(@4, false);

         update_connection(ptr->connection);
         if (@4[0] == ':')
             @$ = cat_str(4, @1, @2, @3, "$0");
-ECPG: fetch_argsABSOLUTE_PSignedIconstopt_from_incursor_name addon
-ECPG: fetch_argsRELATIVE_PSignedIconstopt_from_incursor_name addon
-ECPG: fetch_argsFORWARDSignedIconstopt_from_incursor_name addon
-ECPG: fetch_argsBACKWARDSignedIconstopt_from_incursor_name addon
+ECPG: addon fetch_args ABSOLUTE_P SignedIconst opt_from_in cursor_name
+ECPG: addon fetch_args RELATIVE_P SignedIconst opt_from_in cursor_name
+ECPG: addon fetch_args FORWARD SignedIconst opt_from_in cursor_name
+ECPG: addon fetch_args BACKWARD SignedIconst opt_from_in cursor_name
         struct cursor *ptr = add_additional_variables(@4, false);
         bool    replace = false;

@@ -312,7 +312,7 @@ ECPG: fetch_argsBACKWARDSignedIconstopt_from_incursor_name addon
         }
         if (replace)
             @$ = cat_str(4, @1, @2, @3, @4);
-ECPG: cursor_namename block
+ECPG: block cursor_name name
     | char_civar
     {
         char       *curname = loc_alloc(strlen(@1) + 2);
@@ -320,11 +320,11 @@ ECPG: cursor_namename block
         sprintf(curname, ":%s", @1);
         @$ = curname;
     }
-ECPG: ExplainableStmtExecuteStmt block
+ECPG: block ExplainableStmt ExecuteStmt
     {
         @$ = $1.name;
     }
-ECPG: PrepareStmtPREPAREprepared_nameprep_type_clauseASPreparableStmt block
+ECPG: block PrepareStmt PREPARE prepared_name prep_type_clause AS PreparableStmt
     {
         $$.name = @2;
         $$.type = @3;
@@ -336,20 +336,20 @@ ECPG: PrepareStmtPREPAREprepared_nameprep_type_clauseASPreparableStmt block
         $$.type = NULL;
         $$.stmt = @4;
     }
-ECPG: ExecuteStmtEXECUTEprepared_nameexecute_param_clauseexecute_rest block
+ECPG: block ExecuteStmt EXECUTE prepared_name execute_param_clause execute_rest
     {
         $$.name = @2;
         $$.type = @3;
     }
-ECPG: ExecuteStmtCREATEOptTempTABLEcreate_as_targetASEXECUTEprepared_nameexecute_param_clauseopt_with_dataexecute_rest
block
+ECPG: block ExecuteStmt CREATE OptTemp TABLE create_as_target AS EXECUTE prepared_name execute_param_clause
opt_with_dataexecute_rest 
     {
         $$.name = @$;
     }
-ECPG:
ExecuteStmtCREATEOptTempTABLEIF_PNOTEXISTScreate_as_targetASEXECUTEprepared_nameexecute_param_clauseopt_with_dataexecute_rest
block
+ECPG: block ExecuteStmt CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS EXECUTE prepared_name
execute_param_clauseopt_with_data execute_rest 
     {
         $$.name = @$;
     }
-ECPG: DeclareCursorStmtDECLAREcursor_namecursor_optionsCURSORopt_holdFORSelectStmt block
+ECPG: block DeclareCursorStmt DECLARE cursor_name cursor_options CURSOR opt_hold FOR SelectStmt
     {
         struct cursor *ptr,
                    *this;
@@ -399,7 +399,7 @@ ECPG: DeclareCursorStmtDECLAREcursor_namecursor_optionsCURSORopt_holdFORSelectSt

         @$ = cat2_str(adjust_outofscope_cursor_vars(this), comment);
     }
-ECPG: ClosePortalStmtCLOSEcursor_name block
+ECPG: block ClosePortalStmt CLOSE cursor_name
     {
         const char *cursor_marker = @2[0] == ':' ? "$0" : @2;
         struct cursor *ptr = NULL;
@@ -414,14 +414,14 @@ ECPG: ClosePortalStmtCLOSEcursor_name block
         }
         @$ = cat2_str("close", cursor_marker);
     }
-ECPG: opt_hold block
+ECPG: block opt_hold
     {
         if (compat == ECPG_COMPAT_INFORMIX_SE && autocommit)
             @$ = "with hold";
         else
             @$ = "";
     }
-ECPG: into_clauseINTOOptTempTableName block
+ECPG: block into_clause INTO OptTempTableName
     {
         FoundInto = 1;
         @$ = cat2_str("into", @2);
@@ -430,15 +430,15 @@ ECPG: into_clauseINTOOptTempTableName block
     {
         @$ = "";
     }
-ECPG: TypenameSimpleTypenameopt_array_bounds block
+ECPG: block Typename SimpleTypename opt_array_bounds
     {
         @$ = cat2_str(@1, $2.str);
     }
-ECPG: TypenameSETOFSimpleTypenameopt_array_bounds block
+ECPG: block Typename SETOF SimpleTypename opt_array_bounds
     {
         @$ = cat_str(3, "setof", @2, $3.str);
     }
-ECPG: opt_array_boundsopt_array_bounds'['']' block
+ECPG: block opt_array_bounds opt_array_bounds '[' ']'
     {
         $$.index1 = $1.index1;
         $$.index2 = $1.index2;
@@ -458,20 +458,20 @@ ECPG: opt_array_boundsopt_array_bounds'['']' block
             $$.index2 = @3;
         $$.str = cat_str(4, $1.str, "[", @3, "]");
     }
-ECPG: opt_array_bounds block
+ECPG: block opt_array_bounds
     {
         $$.index1 = "-1";
         $$.index2 = "-1";
         $$.str = "";
     }
-ECPG: AexprConstNULL_P rule
+ECPG: rule AexprConst NULL_P
     | civar
     | civarind
-ECPG: VariableShowStmtSHOWALL block
+ECPG: block VariableShowStmt SHOW ALL
     {
         mmerror(PARSE_ERROR, ET_ERROR, "SHOW ALL is not implemented");
     }
-ECPG: FetchStmtMOVEfetch_args rule
+ECPG: rule FetchStmt MOVE fetch_args
     | FETCH fetch_args ecpg_fetch_into
     | FETCH FORWARD cursor_name opt_ecpg_fetch_into
     {
@@ -545,9 +545,9 @@ ECPG: FetchStmtMOVEfetch_args rule

         @$ = cat_str(2, "move backward from", cursor_marker);
     }
-ECPG: limit_clauseLIMITselect_limit_value','select_offset_value block
+ECPG: block limit_clause LIMIT select_limit_value ',' select_offset_value
     {
         mmerror(PARSE_ERROR, ET_WARNING, "no longer supported LIMIT #,# syntax passed to server");
     }
-ECPG: SignedIconstIconst rule
+ECPG: rule SignedIconst Iconst
     | civar
diff --git a/src/interfaces/ecpg/preproc/parse.pl b/src/interfaces/ecpg/preproc/parse.pl
index 98d44d4bf2..7999ff267d 100644
--- a/src/interfaces/ecpg/preproc/parse.pl
+++ b/src/interfaces/ecpg/preproc/parse.pl
@@ -69,47 +69,47 @@ my %replace_types = (
     'plassign_equals' => 'ignore',);

 # This hash provides an "ignore" option or substitute expansion for any
-# rule or rule alternative.  The hash key is the same "concattokens" tag
+# rule or rule alternative.  The hash key is the same "tokenlist" tag
 # used for lookup in ecpg.addons.
 my %replace_line = (
     # These entries excise certain keywords from the core keyword lists.
     # Be sure to account for these in ColLabel and related productions.
-    'unreserved_keywordCONNECTION' => 'ignore',
-    'unreserved_keywordCURRENT_P' => 'ignore',
-    'unreserved_keywordDAY_P' => 'ignore',
-    'unreserved_keywordHOUR_P' => 'ignore',
-    'unreserved_keywordINPUT_P' => 'ignore',
-    'unreserved_keywordMINUTE_P' => 'ignore',
-    'unreserved_keywordMONTH_P' => 'ignore',
-    'unreserved_keywordSECOND_P' => 'ignore',
-    'unreserved_keywordYEAR_P' => 'ignore',
-    'col_name_keywordCHAR_P' => 'ignore',
-    'col_name_keywordINT_P' => 'ignore',
-    'col_name_keywordVALUES' => 'ignore',
-    'reserved_keywordTO' => 'ignore',
-    'reserved_keywordUNION' => 'ignore',
+    'unreserved_keyword CONNECTION' => 'ignore',
+    'unreserved_keyword CURRENT_P' => 'ignore',
+    'unreserved_keyword DAY_P' => 'ignore',
+    'unreserved_keyword HOUR_P' => 'ignore',
+    'unreserved_keyword INPUT_P' => 'ignore',
+    'unreserved_keyword MINUTE_P' => 'ignore',
+    'unreserved_keyword MONTH_P' => 'ignore',
+    'unreserved_keyword SECOND_P' => 'ignore',
+    'unreserved_keyword YEAR_P' => 'ignore',
+    'col_name_keyword CHAR_P' => 'ignore',
+    'col_name_keyword INT_P' => 'ignore',
+    'col_name_keyword VALUES' => 'ignore',
+    'reserved_keyword TO' => 'ignore',
+    'reserved_keyword UNION' => 'ignore',

     # some other production rules have to be ignored or replaced
-    'fetch_argsFORWARDopt_from_incursor_name' => 'ignore',
-    'fetch_argsBACKWARDopt_from_incursor_name' => 'ignore',
-    "opt_array_boundsopt_array_bounds'['Iconst']'" => 'ignore',
-    'VariableShowStmtSHOWvar_name' => 'SHOW var_name ecpg_into',
-    'VariableShowStmtSHOWTIMEZONE' => 'SHOW TIME ZONE ecpg_into',
-    'VariableShowStmtSHOWTRANSACTIONISOLATIONLEVEL' =>
+    'fetch_args FORWARD opt_from_in cursor_name' => 'ignore',
+    'fetch_args BACKWARD opt_from_in cursor_name' => 'ignore',
+    "opt_array_bounds opt_array_bounds '[' Iconst ']'" => 'ignore',
+    'VariableShowStmt SHOW var_name' => 'SHOW var_name ecpg_into',
+    'VariableShowStmt SHOW TIME ZONE' => 'SHOW TIME ZONE ecpg_into',
+    'VariableShowStmt SHOW TRANSACTION ISOLATION LEVEL' =>
       'SHOW TRANSACTION ISOLATION LEVEL ecpg_into',
-    'VariableShowStmtSHOWSESSIONAUTHORIZATION' =>
+    'VariableShowStmt SHOW SESSION AUTHORIZATION' =>
       'SHOW SESSION AUTHORIZATION ecpg_into',
-    'returning_clauseRETURNINGtarget_list' =>
+    'returning_clause RETURNING target_list' =>
       'RETURNING target_list opt_ecpg_into',
-    'ExecuteStmtEXECUTEnameexecute_param_clause' =>
+    'ExecuteStmt EXECUTE name execute_param_clause' =>
       'EXECUTE prepared_name execute_param_clause execute_rest',
-    'ExecuteStmtCREATEOptTempTABLEcreate_as_targetASEXECUTEnameexecute_param_clauseopt_with_data'
+    'ExecuteStmt CREATE OptTemp TABLE create_as_target AS EXECUTE name execute_param_clause opt_with_data'
       => 'CREATE OptTemp TABLE create_as_target AS EXECUTE prepared_name execute_param_clause opt_with_data
execute_rest',
-    'ExecuteStmtCREATEOptTempTABLEIF_PNOTEXISTScreate_as_targetASEXECUTEnameexecute_param_clauseopt_with_data'
+    'ExecuteStmt CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS EXECUTE name execute_param_clause
opt_with_data'
       => 'CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS EXECUTE prepared_name execute_param_clause
opt_with_dataexecute_rest', 
-    'PrepareStmtPREPAREnameprep_type_clauseASPreparableStmt' =>
+    'PrepareStmt PREPARE name prep_type_clause AS PreparableStmt' =>
       'PREPARE prepared_name prep_type_clause AS PreparableStmt',
-    'var_nameColId' => 'ECPGColId');
+    'var_name ColId' => 'ECPGColId');


 # Declare assorted state variables.
@@ -611,8 +611,9 @@ sub emit_default_action
 sub emit_rule
 {
     # compute tag to be used as lookup key in %replace_line and %addons
-    my $tag = $non_term_id . $line;
-    $tag =~ tr/ |//d;
+    my $tag = $non_term_id . ' ' . $line;
+    $tag =~ tr/|//d;
+    $tag = join(' ', split(/\s+/, $tag));

     # apply replace_line substitution if any
     my $rep = $replace_line{$tag};
@@ -635,8 +636,9 @@ sub emit_rule
         }

         # recompute tag for use in emit_rule_action
-        $tag = $non_term_id . $line;
-        $tag =~ tr/ |//d;
+        $tag = $non_term_id . ' ' . $line;
+        $tag =~ tr/|//d;
+        $tag = join(' ', split(/\s+/, $tag));
     }

     # Emit $line, then print the appropriate action.
@@ -648,8 +650,8 @@ sub emit_rule
 =top
     load ecpg.addons into %addons hash.  The result is something like
     %addons = {
-        stmtClosePortalStmt => { 'type' => 'block', 'lines' => [ "{", "if (INFORMIX_MODE)" ..., "}" ], 'used' => 0 },
-        stmtViewStmt => { 'type' => 'rule', 'lines' => [ "| ECPGAllocateDescr", ... ], 'used' => 0 }
+        'stmt ClosePortalStmt' => { 'type' => 'block', 'lines' => [ "{", "if (INFORMIX_MODE)" ..., "}" ], 'used' => 0
},
+        'stmt ViewStmt' => { 'type' => 'rule', 'lines' => [ "| ECPGAllocateDescr", ... ], 'used' => 0 }
     }

 =cut
@@ -668,14 +670,21 @@ sub preload_addons
     my $skip = 1;
     while (<$fh>)
     {
-        if (/^ECPG:\s+(\S+)\s+(\w+)\s*$/)
+        if (/^ECPG:\s+(\w+)\s+(.*)$/)
         {
             # Found an "ECPG:" line, so we're done skipping the header
             $skip = 0;
+            my $type = $1;
+            my $target = $2;
+            # Normalize target so there's exactly one space between tokens
+            $target = join(' ', split(/\s+/, $target));
             # Validate record type and target
-            die "invalid record type $2 in addon rule for $1\n"
-              unless ($2 eq 'block' or $2 eq 'addon' or $2 eq 'rule');
-            die "duplicate addon rule for $1\n" if (exists $addons{$1});
+            die "invalid record type $type in addon rule for $target\n"
+              unless ($type eq 'block'
+                or $type eq 'addon'
+                or $type eq 'rule');
+            die "duplicate addon rule for $target\n"
+              if (exists $addons{$target});
             # If we had some preceding code lines, attach them to all
             # as-yet-unfinished records.
             if (@code)
@@ -688,10 +697,10 @@ sub preload_addons
                 @needsRules = ();
             }
             my $record = {};
-            $record->{type} = $2;
+            $record->{type} = $type;
             $record->{lines} = [];
             $record->{used} = 0;
-            $addons{$1} = $record;
+            $addons{$target} = $record;
             push(@needsRules, $record);
         }
         elsif (/^ECPG:/)

Re: Improving the notation for ecpg.addons rules

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> It looks like %replace_line expects all its elements to have one space
> between each token, still this is not enforced with a check across its
> hardcoded elements?

Yeah, I was wondering about that.  I wouldn't do it exactly like
that, but with a check that the entry gets matched somewhere.
The other patchset adds cross-checks that each ecpg.addons entry is
used exactly once, but there's no such check for these arrays that
are internal to parse.pl.  Can't quite decide if it's worth adding.

I debugged the patch in this thread by checking that the emitted
preproc.y didn't change, but that might not be helpful for changes
that are actually intended to change the grammar.

            regards, tom lane



Re: Improving the notation for ecpg.addons rules

From
Tom Lane
Date:
I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> It looks like %replace_line expects all its elements to have one space
>> between each token, still this is not enforced with a check across its
>> hardcoded elements?

> Yeah, I was wondering about that.  I wouldn't do it exactly like
> that, but with a check that the entry gets matched somewhere.

Here's a patch for that (again based on the other patch series).
This did not turn up anything interesting, but it's probably
worth keeping.

            regards, tom lane

diff --git a/src/interfaces/ecpg/preproc/parse.pl b/src/interfaces/ecpg/preproc/parse.pl
index 98d44d4bf2..998822ce73 100644
--- a/src/interfaces/ecpg/preproc/parse.pl
+++ b/src/interfaces/ecpg/preproc/parse.pl
@@ -33,7 +33,9 @@ GetOptions(


 # These hash tables define additional transformations to apply to
-# grammar rules.
+# grammar rules.  For bug-detection purposes, we count usages of
+# each hash table entry in a second hash table, and verify that
+# all the entries get used.

 # Substitutions to apply to tokens whenever they are seen in a rule.
 my %replace_token = (
@@ -44,6 +46,8 @@ my %replace_token = (
     'IDENT' => 'ecpg_ident',
     'PARAM' => 'ecpg_param',);

+my %replace_token_used;
+
 # This hash can provide a result type to override "void" for nonterminals
 # that need that, or it can specify 'ignore' to cause us to skip the rule
 # for that nonterminal.  (In either case, ecpg.trailer had better provide
@@ -68,6 +72,8 @@ my %replace_types = (
     'plassign_target' => 'ignore',
     'plassign_equals' => 'ignore',);

+my %replace_types_used;
+
 # This hash provides an "ignore" option or substitute expansion for any
 # rule or rule alternative.  The hash key is the same "concattokens" tag
 # used for lookup in ecpg.addons.
@@ -111,6 +117,8 @@ my %replace_line = (
       'PREPARE prepared_name prep_type_clause AS PreparableStmt',
     'var_nameColId' => 'ECPGColId');

+my %replace_line_used;
+

 # Declare assorted state variables.

@@ -198,6 +206,30 @@ foreach (keys %addons)
     die "addon rule $_ was matched multiple times\n" if $addons{$_}{used} > 1;
 }

+# Likewise cross-check that entries in our internal hash tables match something.
+foreach (keys %replace_token)
+{
+    die "replace_token entry $_ was never used\n"
+      if !defined($replace_token_used{$_});
+    # multiple use of a replace_token entry is fine
+}
+
+foreach (keys %replace_types)
+{
+    die "replace_types entry $_ was never used\n"
+      if !defined($replace_types_used{$_});
+    die "replace_types entry $_ was matched multiple times\n"
+      if $replace_types_used{$_} > 1;
+}
+
+foreach (keys %replace_line)
+{
+    die "replace_line entry $_ was never used\n"
+      if !defined($replace_line_used{$_});
+    die "replace_line entry $_ was matched multiple times\n"
+      if $replace_line_used{$_} > 1;
+}
+

 # Read the backend grammar.
 sub main
@@ -400,6 +432,7 @@ sub main
             # Apply replace_token substitution if we have one.
             if (exists $replace_token{ $arr[$fieldIndexer] })
             {
+                $replace_token_used{ $arr[$fieldIndexer] }++;
                 $arr[$fieldIndexer] = $replace_token{ $arr[$fieldIndexer] };
             }

@@ -425,6 +458,7 @@ sub main
                     && $replace_types{$non_term_id} eq 'ignore')
                 {
                     # We'll ignore this nonterminal and rule altogether.
+                    $replace_types_used{$non_term_id}++;
                     $copymode = 0;
                     next line;
                 }
@@ -451,6 +485,7 @@ sub main
                       . $replace_types{$non_term_id} . ' '
                       . $non_term_id;
                     add_to_buffer('types', $tstr);
+                    $replace_types_used{$non_term_id}++;
                 }

                 # Emit the target part of the rule.
@@ -616,8 +651,10 @@ sub emit_rule

     # apply replace_line substitution if any
     my $rep = $replace_line{$tag};
-    if ($rep)
+    if (defined $rep)
     {
+        $replace_line_used{$tag}++;
+
         if ($rep eq 'ignore')
         {
             return 0;

Re: Improving the notation for ecpg.addons rules

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> The patch does not apply on HEAD due to the dependency with the other
> things you are proposing, and I would have hardcoded failures to check
> that the reports are correct, but that looks neat on read.

I did test it by injecting errors, but I don't see why we'd leave
those in place?

Anyway, my thought was to merge this into the other patch series,
but I didn't do that yet.

            regards, tom lane