Re: Few bison issues

[Akim Demaille]

Le 6 févr. 2013 à 11:59, address@hidden a écrit :

> Hi,

Hi Daniel!

> I found few issues in bison:
> 1. yy::position::columns() will give incorrect result when column + count < 
> 0, because it operates on unsigned values. This was broken when this method 
> was changed to use std::max(). You can fix this by casting column to int type.

Thanks for the report!  Could you please check this proposal?
In particular, I'm not sure I found the right name :)

commit c0517afba4427e3d03272d7b394510634c6c5074
Author: Akim Demaille <address@hidden>
Date:   Thu Apr 11 09:53:18 2013 +0200

    c++: fix several issues with locations
    
    Reported by Daniel Frużyński.
    http://lists.gnu.org/archive/html/bug-bison/2013-02/msg00000.html
    
    * data/location.cc (position::columns, position::lines): Check for
    underflow.
    Fix some weird function signatures.
    (location): Accept signed integers as arguments where appropriate.
    Add operator- and operator+=.
    * doc/bison.texi (C++ position, C++ location): Various fixes
    and completion.
    * tests/c++.at (C++ Locations): New tests.

diff --git a/NEWS b/NEWS
index d7d89d2..cf118c1 100644
--- a/NEWS
+++ b/NEWS
@@ -569,6 +569,11 @@ GNU Bison NEWS
       ...
     }
 
+*** C++ locations
+
+  There are operator- and operator-= for 'location'.  Negative line/column
+  increments can no longer underflow the resulting value.
+
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
 ** Bug fixes
diff --git a/THANKS b/THANKS
index a7d1d47..320e26f 100644
--- a/THANKS
+++ b/THANKS
@@ -29,6 +29,7 @@ Cris Bailiff              address@hidden
 Cris van Pelt             address@hidden
 Csaba Raduly              address@hidden
 Dagobert Michelsen        address@hidden
+Daniel Frużyński          address@hidden
 Daniel Hagerty            address@hidden
 David J. MacKenzie        address@hidden
 David Kastrup             address@hidden
diff --git a/data/location.cc b/data/location.cc
index 987843b..bff75fe 100644
--- a/data/location.cc
+++ b/data/location.cc
@@ -53,14 +53,23 @@ m4_define([b4_position_define],
     /// (line related) Advance to the COUNT next lines.
     void lines (int count = 1)
     {
-      column = ]b4_location_initial_column[u;
-      line += count;
+      if (count)
+        {
+          column = ]b4_location_initial_column[u;
+          line =
+            0 < count || -count < line
+            ? line + count
+            : ]b4_location_initial_line[;
+        }
     }
 
     /// (column related) Advance to the COUNT next columns.
     void columns (int count = 1)
     {
-      column = std::max (]b4_location_initial_column[u, column + count);
+      column =
+        0 < count || -count < column
+        ? column + count
+        : ]b4_location_initial_column[;
     }
     /** \} */
 
@@ -74,15 +83,15 @@ m4_define([b4_position_define],
 
   /// Add and assign a position.
   inline position&
-  operator+= (position& res, const int width)
+  operator+= (position& res, int width)
   {
     res.columns (width);
     return res;
   }
 
   /// Add two position objects.
-  inline const position
-  operator+ (const position& begin, const int width)
+  inline position
+  operator+ (const position& begin, int width)
   {
     position res = begin;
     return res += width;
@@ -90,14 +99,14 @@ m4_define([b4_position_define],
 
   /// Add and assign a position.
   inline position&
-  operator-= (position& res, const int width)
+  operator-= (position& res, int width)
   {
     return res += -width;
   }
 
   /// Add two position objects.
-  inline const position
-  operator- (const position& begin, const int width)
+  inline position
+  operator- (const position& begin, int width)
   {
     return begin + -width;
   }
@@ -186,13 +195,13 @@ m4_define([b4_location_define],
     }
 
     /// Extend the current location to the COUNT next columns.
-    void columns (unsigned int count = 1)
+    void columns (int count = 1)
     {
       end += count;
     }
 
     /// Extend the current location to the COUNT next lines.
-    void lines (unsigned int count = 1)
+    void lines (int count = 1)
     {
       end.lines (count);
     }
@@ -207,27 +216,39 @@ m4_define([b4_location_define],
   };
 
   /// Join two location objects to create a location.
-  inline const location operator+ (const location& begin, const location& end)
+  inline location operator+ (const location& begin, const location& end)
   {
     location res = begin;
     res.end = end.end;
     return res;
   }
 
-  /// Add two location objects.
-  inline const location operator+ (const location& begin, unsigned int width)
+  /// Change end position.
+  inline location operator+ (const location& begin, int width)
   {
     location res = begin;
     res.columns (width);
     return res;
   }
 
-  /// Add and assign a location.
-  inline location& operator+= (location& res, unsigned int width)
+  /// Change end position in place.
+  inline location& operator+= (location& res, int width)
   {
     res.columns (width);
     return res;
   }
+
+  /// Change end position.
+  inline location operator- (const location& begin, int width)
+  {
+    return begin + -width;
+  }
+
+  /// Change end position in place.
+  inline location& operator-= (location& res, int width)
+  {
+    return res += -width;
+  }
 ]b4_percent_define_flag_if([[define_location_comparison]], [[
   /// Compare two location objects.
   inline bool
diff --git a/doc/bison.texi b/doc/bison.texi
index 250e94d..72847a1 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -10591,16 +10591,18 @@ filename_type "@var{type}"}.
 The line, starting at 1.
 @end deftypeivar
 
address@hidden {position} {uint} lines (int @var{height} = 1)
-Advance by @var{height} lines, resetting the column number.
address@hidden {position} {void} lines (int @var{height} = 1)
+If @var{height} is not null, advance by @var{height} lines, resetting the
+column number.  The resulting line number cannot be less than 1.
 @end deftypemethod
 
 @deftypeivar {position} {uint} column
 The column, starting at 1.
 @end deftypeivar
 
address@hidden {position} {uint} columns (int @var{width} = 1)
-Advance by @var{width} columns, without changing the line number.
address@hidden {position} {void} columns (int @var{width} = 1)
+Advance by @var{width} columns, without changing the line number. The
+resulting column number cannot be less than 1.
 @end deftypemethod
 
 @deftypemethod {position} {position&} operator+= (int @var{width})
@@ -10642,14 +10644,16 @@ Reset the location to an empty range at the given 
values.
 The first, inclusive, position of the range, and the first beyond.
 @end deftypeivar
 
address@hidden {location} {uint} columns (int @var{width} = 1)
address@hidden {location} {uint} lines (int @var{height} = 1)
-Advance the @code{end} position.
address@hidden {location} {void} columns (int @var{width} = 1)
address@hidden {location} {void} lines (int @var{height} = 1)
+Forwarded to the @code{end} position.
 @end deftypemethod
 
 @deftypemethod {location} {location} operator+ (const location& @var{end})
 @deftypemethodx {location} {location} operator+ (int @var{width})
 @deftypemethodx {location} {location} operator+= (int @var{width})
address@hidden {location} {location} operator- (int @var{width})
address@hidden {location} {location} operator-= (int @var{width})
 Various forms of syntactic sugar.
 @end deftypemethod
 
diff --git a/tests/c++.at b/tests/c++.at
index 3300352..387d487 100644
--- a/tests/c++.at
+++ b/tests/c++.at
@@ -18,6 +18,68 @@
 AT_BANNER([[C++ Features.]])
 
 
+## --------------- ##
+## C++ Locations.  ##
+## --------------- ##
+
+AT_SETUP([C++ Locations])
+
+AT_BISON_OPTION_PUSHDEFS([%locations %skeleton "lalr1.cc"])
+AT_DATA_GRAMMAR([[input.y]],
+[[%code {#include <sstream>}
+%locations
+%debug
+%skeleton "lalr1.cc"
+%code
+{
+]AT_YYERROR_DECLARE[
+]AT_YYLEX_DECLARE[
+}
+%%
+exp: %empty;
+%%
+]AT_YYERROR_DEFINE[
+]AT_YYLEX_DEFINE[
+
+template <typename T>
+bool
+check (const T& in, const std::string& s)
+{
+  std::stringstream os;
+  os << in;
+  if (os.str () != s)
+    {
+      std::cerr << "fail: " << os.str () << ", expected: " << s << std::endl;
+      return false;
+    }
+  return true;
+}
+
+int
+main (void)
+{
+  int fail = 0;
+  ]AT_YYLTYPE[ loc;  fail += check (loc, "1.1");
+  loc += 10;         fail += check (loc, "1.1-10");
+  loc += -5;         fail += check (loc, "1.1-5");
+  loc -= 5;          fail += check (loc, "1.1");
+  // Check that we don't go below.
+  // http://lists.gnu.org/archive/html/bug-bison/2013-02/msg00000.html
+  loc -= 10;         fail += check (loc, "1.1");
+
+  loc.columns (10); loc.lines (10); fail += check (loc, "1.1-11.0");
+  loc.lines (-2);                   fail += check (loc, "1.1-9.0");
+  loc.lines (-10);                  fail += check (loc, "1.1");
+  return !fail;
+}
+]])
+
+AT_FULL_COMPILE([input])
+AT_PARSER_CHECK([./input], 0)
+AT_BISON_OPTION_POPDEFS
+AT_CLEANUP
+
+
 ## --------------------------- ##
 ## C++ Variant-based Symbols.  ##
 ## --------------------------- ##