Re: [PATCH for Dlang support 1/2] d: change the return value of yylex from TokenKind to YYParser.Symbol

Adela Vais <[email protected]> Mon, 21 Dec 2020 16:06:36 +0200
Newsgroups gmane.comp.parsers.bison.patches
Message-ID <CAPk8xGes0=YBLnY5sv_Gn2CzQ__e2vUNkyjd4zi1hwhqZ8w4gw@mail.gmail.com>
Hello Akim,

I addressed the suggested modifications and removed the 3 unused methods
from the Lexer.
Thank you for the feedback and help!

Adela

În lun., 21 dec. 2020 la 08:34, Akim Demaille <[email protected]> a scris:

> Hi Adela,
>
> This is a great piece of work!  The overall result is much better,
> well done!
>
> > Le 18 déc. 2020 à 19:37, Adela Vais <[email protected]> a écrit :
> >
> > Hello!
> >
> > I made the suggested modifications.
> >
> > I have a question: given that the user will provide all the needed
> > information through the return value, should semanticVal(), startPos()
> and
> > endPos() still be part of the Lexer interface?
>
> No, they are not useful.  They make sense only with split symbols.
>
> Besides, in that case (split symbols) I personally think the right
> interface is that of locations, not that of positions.  IMHO it was
> an error in Java to put forward positions, and leave locations to
> the parser only.  C and C++ show only location-based interfaces.
> It's up to the user to decide that a location is nothing but a
> position if she thinks two positions is too costly (but that would
> be mean to the users).
>
>
> > În vin., 20 nov. 2020 la 20:18, Akim Demaille <[email protected]> a
> scris:
> >
> >> You should introduce type aliases for b4_yystype and YYLocation.
> >> In the C++ parser, you have value_type and location_type which
> >> are defined to whatever they are actually.  The code is nicer
> >> to read, with fewer occurrences of ugly YYnames.
> >>
> >>
> > I named them Location and Value.
>
> Excellent.
>
> > Should I also change b4_location_type to
> > the new Location alias throughout lalr1.d? I know that the user should
> not
> > use yy names (and before these commits, the examples used YYLocation)
> but I
> > don't know how much of the backend I should change. I changed the
> > b4_yystype occurrences.
>
> I would also use Location everywhere.
>
>
> >>>        if (yychar == TokenKind.]b4_symbol(empty, id)[)
> >>>        {]b4_parse_trace_if([[
> >>>          yycdebugln ("Reading a token");]])[
> >>> -          yychar = yylex ();]b4_locations_if([[
> >>> -          static if (yy_location_is_class) {
> >>> -            yylloc = new ]b4_location_type[(yylexer.startPos,
> >> yylexer.endPos);
> >>> -          } else {
> >>> -            yylloc = ]b4_location_type[(yylexer.startPos,
> >> yylexer.endPos);
> >>> -          }]])
> >>> -          yylval = yylexer.semanticVal;[
> >>> +          Symbol yysymbol = yylex();
> >>> +          yychar = yysymbol.token();
> >>
> >> Maybe you don't need yychar, but only need yytoken.  You probably
> >> can avoid dealing with the token-kinds here, and deal only with
> >> the symbol kinds.
> >>
> > Done.
>
> Great.  This part, however, is too complex and not very clear:
>
> > -          if (yychar <= TokenKind.]b4_symbol(eof, [id])[)
> > +          if (yytoken <= ]b4_symbol(eof, [kind])[)
> >            {
> >              /* Return failure if at end of input.  */
> > -            if (yychar == TokenKind.]b4_symbol(eof, [id])[)
> > +            if (yytoken == ]b4_symbol(eof, [kind])[)
> >               return false;
> >            }
> >            else
> > -            yychar = TokenKind.]b4_symbol(empty, id)[;
> > +            yytoken = ]b4_symbol(empty, kind)[;
>
> When accepting token kinds as return values from yylex, we tolerate
> any nonpositive number to denote EOF.  Hence the first if.  However
> when using symbol kinds, we should not accept any deviation from
> the symbols kinds.  So that should be a single check "== eof", not
> two.
>
> Maybe have a look at what was done in C++.
>
> Again, great work, thanks!
>
> Cheers!
>
>
0001-d-remove-unnecessary-comparison-from-YYParser.parse.patch (application/octet-stream, 1.1 KB)
From 05c5782227f00f31bcdb6c48a7b5a1b0c1625335 Mon Sep 17 00:00:00 2001
From: Adela Vais <[email protected]>
Date: Mon, 21 Dec 2020 13:38:00 +0200
Subject: [PATCH for Dlang support 1/3] d: remove unnecessary comparison from
 YYParser.parse()

* data/skeletons/lalr1.d: Here.
---
 data/skeletons/lalr1.d | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/data/skeletons/lalr1.d b/data/skeletons/lalr1.d
index 4a2dff38..2b81514c 100644
--- a/data/skeletons/lalr1.d
+++ b/data/skeletons/lalr1.d
@@ -591,12 +591,9 @@ m4_popdef([b4_at_dollar])])dnl
           /* If just tried and failed to reuse lookahead token after an
            * error, discard it.  */
 
-          if (yytoken <= ]b4_symbol(eof, [kind])[)
-          {
-            /* Return failure if at end of input.  */
-            if (yytoken == ]b4_symbol(eof, [kind])[)
-             return false;
-          }
+          /* Return failure if at end of input.  */
+          if (yytoken == ]b4_symbol(eof, [kind])[)
+            return false;
           else
             yytoken = ]b4_symbol(empty, kind)[;
         }
-- 
2.17.1
0003-d-remove-unnecessary-methods-from-the-Lexer-interfac.patch (application/octet-stream, 5 KB)
From fd59f01723a21edfc396c02120c67cbf3f8022ca Mon Sep 17 00:00:00 2001
From: Adela Vais <[email protected]>
Date: Mon, 21 Dec 2020 15:49:27 +0200
Subject: [PATCH for Dlang support 3/3] d: remove unnecessary methods from the
 Lexer interface

The complete symbol approach in yylex removes the need for the methods
semanticVal, startPos and endPos, which were used when the values were
reported separately.

* data/skeletons/lalr1.d: Here.
* doc/bison.texi: Remove sections about the three methods.
* examples/d/calc/calc.y, examples/d/simple/calc.y: Remove the unused methods.
* tests/calc.at, tests/d.at, tests/scanner.at: Test it.
---
 data/skeletons/lalr1.d   | 18 +-----------------
 doc/bison.texi           | 10 ----------
 examples/d/calc/calc.y   | 15 ---------------
 examples/d/simple/calc.y |  7 -------
 tests/calc.at            | 15 ---------------
 tests/d.at               |  1 -
 tests/scanner.at         |  4 ----
 7 files changed, 1 insertion(+), 69 deletions(-)

diff --git a/data/skeletons/lalr1.d b/data/skeletons/lalr1.d
index 7fe656d3..fd0038d6 100644
--- a/data/skeletons/lalr1.d
+++ b/data/skeletons/lalr1.d
@@ -53,23 +53,7 @@ import std.format;
  * parser <tt>]b4_parser_class[</tt>.
  */
 public interface Lexer
-{]b4_locations_if([[
-  /**
-   * Method to retrieve the beginning position of the last scanned token.
-   * @@return the position at which the last scanned token starts.  */
-  Position startPos ();
-
-  /**
-   * Method to retrieve the ending position of the last scanned token.
-   * @@return the first position beyond the last scanned token.  */
-  Position endPos ();
-
-]])[
-  /**
-   * Method to retrieve the semantic value of the last scanned token.
-   * @@return the semantic value of the last scanned token.  */
-  Value semanticVal ();
-
+{
   /**
    * Entry point for the scanner.  Returns the token identifier corresponding
    * to the next token and prepares to return the semantic value
diff --git a/doc/bison.texi b/doc/bison.texi
index df169517..a272b387 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -14016,16 +14016,6 @@ Return the next token. The return value is of type @code{Symbol}, which
 binds together the kind, the semantic value and the location.
 @end deftypemethod
 
-@deftypemethod {Lexer} {Position} getStartPos()
-@deftypemethodx {Lexer} {Position} getEndPos()
-Return respectively the first position of the last token that @code{yylex}
-returned, and the first position beyond it.  These methods are not needed
-unless location tracking is active.
-
-They should return new objects for each call, to avoid that all the symbol
-share the same Position boundaries.
-@end deftypemethod
-
 @deftypemethod {Lexer} {void} reportSyntaxError(@code{YYParser.Context} @var{ctx})
 If you invoke @samp{%define parse.error custom} (@pxref{Bison
 Declarations}), then the parser no longer passes syntax error messages to
diff --git a/examples/d/calc/calc.y b/examples/d/calc/calc.y
index 9599501f..2c79c158 100644
--- a/examples/d/calc/calc.y
+++ b/examples/d/calc/calc.y
@@ -108,11 +108,6 @@ if (isInputRange!R && is(ElementType!R : dchar))
 
   Value semanticVal_;
 
-  public final Value semanticVal()
-  {
-    return semanticVal_;
-  }
-
   Symbol yylex()
   {
     import std.uni : isWhite, isNumber;
@@ -167,16 +162,6 @@ if (isInputRange!R && is(ElementType!R : dchar))
       default: assert(0);
     }
   }
-
-  Position startPos() const
-  {
-    return location.begin;
-  }
-
-  Position endPos() const
-  {
-    return location.end;
-  }
 }
 
 int main()
diff --git a/examples/d/simple/calc.y b/examples/d/simple/calc.y
index 5fca647e..58f1d020 100644
--- a/examples/d/simple/calc.y
+++ b/examples/d/simple/calc.y
@@ -102,13 +102,6 @@ if (isInputRange!R && is(ElementType!R : dchar))
     stderr.writeln(s);
   }
 
-  Value semanticVal_;
-
-  public final Value semanticVal()
-  {
-    return semanticVal_;
-  }
-
   Symbol yylex()
   {
     import std.uni : isWhite, isNumber;
diff --git a/tests/calc.at b/tests/calc.at
index 75b64e10..de85f04d 100644
--- a/tests/calc.at
+++ b/tests/calc.at
@@ -561,22 +561,7 @@ class CalcLexer(R) : Lexer
 
   Value semanticVal_;]AT_LOCATION_IF([[
   Location location;
-
-  public final @property Position startPos()
-  {
-    return location.begin;
-  }
-
-  public final @property Position endPos()
-  {
-    return location.end;
-  }
 ]])[
-  public final @property Value semanticVal()
-  {
-    return semanticVal_;
-  }
-
   int parseInt ()
   {
     auto res = 0;
diff --git a/tests/d.at b/tests/d.at
index 07de11bc..f268b820 100644
--- a/tests/d.at
+++ b/tests/d.at
@@ -79,7 +79,6 @@ class CalcLexer(R) : Lexer
   void yyerror(string s) {}
 
   Value semanticVal_;
-  Value semanticVal() @property { return semanticVal_; }
 
   Symbol yylex()
   {
diff --git a/tests/scanner.at b/tests/scanner.at
index e55ad3b6..46a20116 100644
--- a/tests/scanner.at
+++ b/tests/scanner.at
@@ -116,10 +116,6 @@ class YYLexer(R) : Lexer
   ]AT_YYERROR_DEFINE[
 
   Value semanticVal_;
-  public final @property Value semanticVal ()
-  {
-    return semanticVal_;
-  }
 
   Symbol yylex ()
   {
-- 
2.17.1
0002-d-use-Location-and-Position-aliases-in-the-backend.patch (application/octet-stream, 9.4 KB)
From c9ab1695d6ad03764b25da0c2078531893a3a562 Mon Sep 17 00:00:00 2001
From: Adela Vais <[email protected]>
Date: Mon, 21 Dec 2020 13:44:33 +0200
Subject: [PATCH for Dlang support 2/3] d: use Location and Position aliases in
 the backend

* data/skeletons/lalr1.d: Here.
---
 data/skeletons/lalr1.d | 82 +++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/data/skeletons/lalr1.d b/data/skeletons/lalr1.d
index 2b81514c..7fe656d3 100644
--- a/data/skeletons/lalr1.d
+++ b/data/skeletons/lalr1.d
@@ -84,7 +84,7 @@ public interface Lexer
    * @@param loc The location of the element to which the
    *                error message is related]])[
    * @@param s The string for the error message.  */
-   void yyerror (]b4_locations_if([[const ]b4_location_type[ loc, ]])[string s);
+   void yyerror (]b4_locations_if([[const Location loc, ]])[string s);
 ]b4_parse_error_bmatch([custom], [[
   /**
    * Build and emit a "syntax error" message in a user-defined way.
@@ -99,11 +99,11 @@ public interface Lexer
 
 ]b4_locations_if([b4_position_type_if([[
 static assert(__traits(compiles,
-              (new ]b4_position_type[[1])[0]=(new ]b4_position_type[[1])[0]),
-              "struct/class ]b4_position_type[ must be default-constructible "
+              (new Position[1])[0]=(new Position[1])[0]),
+              "struct/class Position must be default-constructible "
               "and assignable");
-static assert(__traits(compiles, (new string[1])[0]=(new ]b4_position_type[).toString()),
-              "error: struct/class ]b4_position_type[ must have toString method");
+static assert(__traits(compiles, (new string[1])[0]=(new Position).toString()),
+              "error: struct/class Position must have toString method");
 ]], [[
   /**
    * A struct denoting a point in the input.*/
@@ -126,46 +126,46 @@ public struct ]b4_position_type[ {
   }
 }
 ]])b4_location_type_if([[
-static assert(__traits(compiles, (new ]b4_location_type[((new ]b4_position_type[[1])[0]))) &&
-              __traits(compiles, (new ]b4_location_type[((new ]b4_position_type[[1])[0], (new ]b4_position_type[[1])[0]))),
-              "error: struct/class ]b4_location_type[ must have "
-              "default constructor and constructors this(]b4_position_type[) and this(]b4_position_type[, ]b4_position_type[).");
-static assert(__traits(compiles, (new ]b4_location_type[[1])[0].begin=(new ]b4_location_type[[1])[0].begin) &&
-              __traits(compiles, (new ]b4_location_type[[1])[0].begin=(new ]b4_location_type[[1])[0].end) &&
-              __traits(compiles, (new ]b4_location_type[[1])[0].end=(new ]b4_location_type[[1])[0].begin) &&
-              __traits(compiles, (new ]b4_location_type[[1])[0].end=(new ]b4_location_type[[1])[0].end),
-              "error: struct/class ]b4_location_type[ must have assignment-compatible "
+static assert(__traits(compiles, (new Location((new Position[1])[0]))) &&
+              __traits(compiles, (new Location((new Position[1])[0], (new Position[1])[0]))),
+              "error: struct/class Location must have "
+              "default constructor and constructors this(Position) and this(Position, Position).");
+static assert(__traits(compiles, (new Location[1])[0].begin=(new Location[1])[0].begin) &&
+              __traits(compiles, (new Location[1])[0].begin=(new Location[1])[0].end) &&
+              __traits(compiles, (new Location[1])[0].end=(new Location[1])[0].begin) &&
+              __traits(compiles, (new Location[1])[0].end=(new Location[1])[0].end),
+              "error: struct/class Location must have assignment-compatible "
               "members/properties 'begin' and 'end'.");
-static assert(__traits(compiles, (new string[1])[0]=(new ]b4_location_type[[1])[0].toString()),
-              "error: struct/class ]b4_location_type[ must have toString method.");
+static assert(__traits(compiles, (new string[1])[0]=(new Location[1])[0].toString()),
+              "error: struct/class Location must have toString method.");
 
-private immutable bool yy_location_is_class = !__traits(compiles, *(new ]b4_location_type[((new ]b4_position_type[[1])[0])));]], [[
+private immutable bool yy_location_is_class = !__traits(compiles, *(new Location((new Position[1])[0])));]], [[
 /**
  * A struct defining a pair of positions.  Positions, defined by the
- * <code>]b4_position_type[</code> struct, denote a point in the input.
+ * <code>Position</code> struct, denote a point in the input.
  * Locations represent a part of the input through the beginning
  * and ending positions.  */
 public struct ]b4_location_type[
 {
   /** The first, inclusive, position in the range.  */
-  public ]b4_position_type[ begin;
+  public Position begin;
 
   /** The first position beyond the range.  */
-  public ]b4_position_type[ end;
+  public Position end;
 
   /**
-   * Create a <code>]b4_location_type[</code> denoting an empty range located at
+   * Create a <code>Location</code> denoting an empty range located at
    * a given point.
    * @@param loc The position at which the range is anchored.  */
-  public this (]b4_position_type[ loc) {
+  public this (Position loc) {
     this.begin = this.end = loc;
   }
 
   /**
-   * Create a <code>]b4_location_type[</code> from the endpoints of the range.
+   * Create a <code>Location</code> from the endpoints of the range.
    * @@param begin The first position included in the range.
    * @@param end   The first position beyond the range.  */
-  public this (]b4_position_type[ begin, ]b4_position_type[ end)
+  public this (Position begin, Position end)
   {
     this.begin = begin;
     this.end = end;
@@ -203,18 +203,18 @@ b4_user_union_members
 ]b4_declare_symbol_enum[
 
 ]b4_locations_if([[
-  private final ]b4_location_type[ yylloc_from_stack (ref YYStack rhs, int n)
+  private final Location yylloc_from_stack (ref YYStack rhs, int n)
   {
     static if (yy_location_is_class) {
       if (n > 0)
-        return new ]b4_location_type[ (rhs.locationAt (n-1).begin, rhs.locationAt (0).end);
+        return new Location (rhs.locationAt (n-1).begin, rhs.locationAt (0).end);
       else
-        return new ]b4_location_type[ (rhs.locationAt (0).end);
+        return new Location (rhs.locationAt (0).end);
     } else {
       if (n > 0)
-        return ]b4_location_type[ (rhs.locationAt (n-1).begin, rhs.locationAt (0).end);
+        return Location (rhs.locationAt (n-1).begin, rhs.locationAt (0).end);
       else
-        return ]b4_location_type[ (rhs.locationAt (0).end);
+        return Location (rhs.locationAt (0).end);
     }
   }]])[
 
@@ -292,7 +292,7 @@ b4_user_union_members
     return yylexer.yylex ();
   }
 
-  protected final void yyerror (]b4_locations_if([[const ]b4_location_type[ loc, ]])[string s) {
+  protected final void yyerror (]b4_locations_if([[const Location loc, ]])[string s) {
     yylexer.yyerror (]b4_locations_if([loc, ])[s);
   }
 
@@ -352,7 +352,7 @@ b4_user_union_members
   private int yyaction (int yyn, ref YYStack yystack, int yylen)
   {
     Value yyval;]b4_locations_if([[
-    ]b4_location_type[ yyloc = yylloc_from_stack (yystack, yylen);]])[
+    Location yyloc = yylloc_from_stack (yystack, yylen);]])[
 
     /* If YYLEN is nonzero, implement the default value of the action:
        `$$ = $1'.  Otherwise, use the top of the stack.
@@ -394,7 +394,7 @@ b4_user_union_members
 
   private final void yy_symbol_print (string s, SymbolKind yykind,
     ref Value yyvaluep]dnl
-b4_locations_if([, ref ]b4_location_type[ yylocationp])[)
+b4_locations_if([, ref Location yylocationp])[)
   {
     if (0 < yydebug)
     {
@@ -433,13 +433,13 @@ b4_locations_if([, ref ]b4_location_type[ yylocationp])[)
     /* Error handling.  */
     int yynerrs_ = 0;]b4_locations_if([[
     /// The location where the error started.
-    ]b4_location_type[ yyerrloc;
+    Location yyerrloc;
 
-    /// ]b4_location_type[ of the lookahead.
-    ]b4_location_type[ yylloc;
+    /// Location of the lookahead.
+    Location yylloc;
 
     /// @@$.
-    ]b4_location_type[ yyloc;]])[
+    Location yyloc;]])[
 
     /// Semantic value of the lookahead.
     Value yylval;
@@ -763,9 +763,9 @@ m4_popdef([b4_at_dollar])])dnl
     private ]b4_parser_class[ yyparser;]])[
     private const(YYStack) yystack;
     private SymbolKind yytoken;]b4_locations_if([[
-    private const(]b4_location_type[) yylocation;]])[
+    private const(Location) yylocation;]])[
 
-    this(]b4_lac_if([[]b4_parser_class[ parser, ]])[YYStack stack, SymbolKind kind]b4_locations_if([[, ]b4_location_type[ loc]])[)
+    this(]b4_lac_if([[]b4_parser_class[ parser, ]])[YYStack stack, SymbolKind kind]b4_locations_if([[, Location loc]])[)
     {]b4_lac_if([[
         yyparser = parser;]])[
       yystack = stack;
@@ -778,7 +778,7 @@ m4_popdef([b4_at_dollar])])dnl
       return yytoken;
     }]b4_locations_if([[
 
-    final const(]b4_location_type[) getLocation() const
+    final const(Location) getLocation() const
     {
       return yylocation;
     }]])[
@@ -1098,7 +1098,7 @@ m4_popdef([b4_at_dollar])])dnl
     }
 
     public final void push (int state, Value value]dnl
-  b4_locations_if([, ref ]b4_location_type[ loc])[)
+  b4_locations_if([, ref Location loc])[)
     {
       stack ~= YYStackElement(state, value]b4_locations_if([, loc])[);
     }
@@ -1119,7 +1119,7 @@ m4_popdef([b4_at_dollar])])dnl
     }
 
 ]b4_locations_if([[
-    public final ref ]b4_location_type[ locationAt (int i)
+    public final ref Location locationAt (int i)
     {
       return stack[$-i-1].location;
     }]])[
-- 
2.17.1