[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] grammar: new syntax allowing for partial order symbol preced

From: Valentin Tolmer
Subject: Re: [PATCH] grammar: new syntax allowing for partial order symbol precedence
Date: Wed, 03 Jul 2013 18:44:51 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130623 Thunderbird/17.0.7

Hi, thanks for the quick review.

I confess I prefer using < in general, but I guess this way
it makes your life easier when dealing with the handling of
"<" and "<tag>".
I did not think about that.
Note that current system presents precedences in the increasing
order, the opposite of yours.
Not quite. In my system, if a > b, then the rule with a will be applied before b, and in the older one, a's prec number is higher than b's number.

@@ -263,66 +276,72 @@ resolve_sr_conflict (state *s, int ruleno, symbol 
**errors, int *nerrs)
   reductions *reds = s->reductions;
   /* Find the rule to reduce by to get precedence of reduction.  */
   rule *redrule = reds->rules[ruleno];
-  int redprec = redrule->prec->prec;
+  prec_node * redprecsym = redrule->prec->prec_node;

prec_node *redprecsym

   bitset lookahead_tokens = reds->lookahead_tokens[ruleno];

   for (i = 0; i < ntokens; i++)
     if (bitset_test (lookahead_tokens, i)
-        && bitset_test (lookahead_set, i)
-        && symbols[i]->prec)
+        && bitset_test (lookahead_set, i))
-        /* Shift-reduce conflict occurs for token number i
-           and it has a precedence.
-           The precedence of shifting is that of token i.  */
-        if (symbols[i]->prec < redprec)
-          {
-            register_precedence (redrule->prec->number, i);
-            log_resolution (redrule, i, reduce_resolution);
-            flush_shift (s, i);
-          }
-        else if (symbols[i]->prec > redprec)
+        if (redprecsym && symbols[i]->prec_node)
-            register_precedence (i, redrule->prec->number);
-            log_resolution (redrule, i, shift_resolution);
-            flush_reduce (lookahead_tokens, i);
+            /* Shift-reduce conflict occurs for token number i
+               and it has a precedence.
+               The precedence of shifting is that of token i.  */
+            if (is_prec_superior (redprecsym, symbols[i]->prec_node))
+              {
+                register_precedence (redrule->prec->number, i);
+                log_resolution (redrule, i, reduce_resolution);
+                flush_shift (s, i);
+              }
+            else if (is_prec_superior (symbols[i]->prec_node, redprecsym))
+              {
+                register_precedence (i, redrule->prec->number);
+                log_resolution (redrule, i, shift_resolution);
+                flush_reduce (lookahead_tokens, i);
+              }
+            else if (is_prec_equal (redprecsym, symbols[i]->prec_node))
+              /* Matching precedence levels.
+                 For non-defined associativity, keep both: unexpected
+                 associativity conflict.
+                 For left associativity, keep only the reduction.
+                 For right associativity, keep only the shift.
+                 For nonassociativity, keep neither.  */
+              switch (symbols[i]->assoc)
+                {
+                case undef_assoc:
+                  abort ();
+                case precedence_assoc:
+                  break;
+                case right_assoc:
+                  register_assoc (i, redrule->prec->number);
+                  log_resolution (redrule, i, right_resolution);
+                  flush_reduce (lookahead_tokens, i);
+                  break;
+                case left_assoc:
+                  register_assoc (i, redrule->prec->number);
+                  log_resolution (redrule, i, left_resolution);
+                  flush_shift (s, i);
+                  break;
+                case non_assoc:
+                  register_assoc (i, redrule->prec->number);
+                  log_resolution (redrule, i, nonassoc_resolution);
+                  flush_shift (s, i);
+                  flush_reduce (lookahead_tokens, i);
+                  /* Record an explicit error for this token.  */
+                  errors[(*nerrs)++] = symbols[i];
+                  break;
+                }
+            else
+              log_resolution (redrule, i, uncomparable_resolution);

Wouldn't it be time to extract a small function in here?

Extracting a function would boil down to simply copy-paste the code out of the for loop, and passing it every local variable and argument, so I don't really think it would make the code simpler.
diff --git a/src/gram.h b/src/gram.h
index c1dd9a6..4dd2a88 100644
--- a/src/gram.h
+++ b/src/gram.h
@@ -117,6 +117,9 @@ typedef int item_number;
extern item_number *ritem;
extern unsigned int nritems;

+/* Marker for the lexer and parser, to correctly interpret braces. */
+static int prec_braces = 0;

What are its values?  Why the magic "& 1"?

I didn't know exactly how to do it, but the values for prec_braces are
0 -> default state
1 -> seen %gprec
3 -> seen %gprec name
4 -> inside the braces
hence the "& 1" to check if we are at the point of opening a brace.
diff --git a/src/scan-gram.l b/src/scan-gram.l
index 665e80d..f89647a 100644
--- a/src/scan-gram.l
+++ b/src/scan-gram.l
@@ -238,6 +238,11 @@ eqopt    ([[:space:]]*=)?
   "%param"                          RETURN_PERCENT_PARAM(both);
   "%parse-param"                    RETURN_PERCENT_PARAM(parse);
   "%prec"                           return PERCENT_PREC;
+  "%gprec" {
+    prec_braces = 1;
+    return PERCENT_GPREC;
+  }

The indentation does not match the remainder of the code.
I don't see how : for every other case with braces blocks, the brace is right after the token and the code has one level of indentation.
+              to->symbol->tag, from->symbol->tag);
+  else if (is_prec_equal (from, to))
+    complain (&loc, fatal,
+              _("Contradicting declaration: %s > %s is in conflict with the \
+previous declaration of %s = %s"), from->symbol->tag, to->symbol->tag,
+              from->symbol->tag, to->symbol->tag);

Using a common message with a %s for = or > would make translators lives
easier.  Introduce a function to issue the error.
Is a macro OK?
| Create a new symbol, named TAG.  |
@@ -93,6 +345,12 @@ symbol_new (uniqstr tag, location loc)
   res->class = unknown_sym;
   res->status = undeclared;

+  res->group_next = NULL;
+  res->prec_node = malloc (sizeof *res->prec_node);
+  res->prec_node->symbol = res;
+  res->prec_node->sons = NULL;
+  res->prec_node->equals = NULL;

Shouldn't you introduce a prec_node_new function?

It's he only place where I create a prec_node, so I didn't deem it necessary.
@@ -541,11 +803,9 @@ symbol_check_alias_consistency (symbol *this)
   if (sym->prec || str->prec)
       if (str->prec)
-        symbol_precedence_set (sym, str->prec, str->assoc,
-                               str->prec_location);
+        sym->assoc = str->assoc;
-        symbol_precedence_set (str, sym->prec, sym->assoc,
-                               sym->prec_location);
+        str->assoc = sym->assoc;

Use ?:.
I can't, it's not the same affectation.

Should be a separated patch.  This patch is doing way too many
things at the same time.

It slipped past my screening, I'll remove it.

+  sprintf (buff, "__anon%i__", anon_group_counter++);

Don't use dangerous functions, use snprintf.  I'm not sure %i
is portable.

How about %u? I'll make the counter unsigned.

Tests are needed!

I'm working on them. This patch doesn't even pass all of bison's checks, I realised. There is a problem with symbol redeclaration : how should I handle the precedence? So far, the attitude was to issue a warning and to overwrite the precedence level, I believe. My patch fails when it tries to make the relation token > itself. So the question is: is that an acceptable behaviour? If not, should I issue a warning and keep the precedence info as is? Or try to reconstitute the precedence as it would have been if the token had instead been declared there (that's going to be VERY hard, I'm afraid).

And I still have a few abort() to root out during the conflict resolution, but I think I know how to do that.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]