Hi Akim,
> If you're allowed modern c++, decltype should provide you with portability
> across versions of Bison.
We don't worry that much about portability across Bison versions.
We actually made sure that we only ever build with an exactly defined bison
version and
that each upgrade needs to be done consciously. As soon as we switch to a newer
bison
version, we will do so atomically for all our builds and will just patch our
code with the
same commit with which we also upgrade our bison dependency.
> Yet we should definitely expose our type in locations.
Yes, that might be useful.
On the other hand, I guess we at Tableau should have written our own location
type instead of abusing
the bison-provided one for storing byte offsets, so - whatever works...
> WDYT about this commit?
In general, I think it's good to switch from unsigned to signed.
And we can easily deal with the fallout, i.e. patching our source code
Cheers,
Adrian
On 06/11/2019, 18:20, "Akim Demaille" <a...@lrde.epita.fr> wrote:
Hi Adrian,
> Le 5 nov. 2019 à 16:40, Adrian Vogelsgesang <avogelsges...@tableau.com> a
écrit :
>
> Hi Akim,
>
> I quickly upgraded Hyper (having a SQL grammar, similar to Postgres’
grammar)
> to use Bison 3.4.90.
> Things are looking good, our internal test cases are successful.
> I did not go through all our tests, though, e.g. I did not check address
sanitizer and friends.
I'd feel safer but I already ran it on the CI, so we should be safe enough
:)
> I also tested LAC, and it works :)
Good :)
>> positions moved from unsigned (for line and column numbers) to int.
> Yes, I ran into this. Our build was failing at first due to this.
>
> …/Debug/codegen/hyper/cts/parser/combinedsql.ypp: In function ‘int
yylex(hyper::astql::AST**, sqlparser::SQLParser::location_type*,
hyper::SQLLexer&, hyper::ASTContainer&)’:
> …/Debug/codegen/hyper/cts/parser/combinedsql.ypp:2504:79: error:
narrowing conversion of ‘(unsigned int)(((((long
int)token.hyper::SQLLexer::Token::begin) - ((long int)scannerBegin)) (ceiling
/) 1) + 1)’ from ‘unsigned int’ to ‘int’ inside { } [-Werror=narrowing]
>
(*yyloc)=sqlparser::SQLParser::location_type{sqlparser::position{nullptr,1,static_cast<unsigned>(token.begin-scannerBegin+1)},sqlparser::position{nullptr,1,static_cast<unsigned>(token.end-scannerBegin+1)}};
>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fortunately, it was easy to fix by changing the static_cast<unsigned> to
static_cast<int>.
> We are abusing the bison-provided location class to store byte-offsets
here, so I guess this falls into the category of “breakage due to API abuse”.
If you're allowed modern c++, decltype should provide you with portability
across versions of Bison. Yet we should definitely expose our type in
locations. WDYT about this commit?
Cheers!
commit 7bdf7246fbc510fd13e4feeb95d568a6b7eac65f
Author: Akim Demaille <akim.demai...@gmail.com>
Date: Wed Nov 6 08:33:44 2019 +0100
c++: expose the type used to store line and column numbers
* data/skeletons/location.cc (position::counter_type)
(location::counter_type): New.
Use them.
* doc/bison.texi (C++ position, C++ location): Adjust.
diff --git a/NEWS b/NEWS
index 441d0fca..8e6aed95 100644
--- a/NEWS
+++ b/NEWS
@@ -14,7 +14,8 @@ GNU Bison NEWS
In C++, line numbers and columns are now represented as 'int' not
'unsigned', so that integer overflow on positions is easily checkable via
'gcc -fsanitize=undefined' and the like. This affects the API for
- positions.
+ positions. The default position and location classes now expose
+ 'counter_type' (int), used to define line and column numbers.
** Bug fixes
diff --git a/data/skeletons/location.cc b/data/skeletons/location.cc
index e544f707..02583798 100644
--- a/data/skeletons/location.cc
+++ b/data/skeletons/location.cc
@@ -62,11 +62,14 @@ m4_define([b4_location_define],
[[ /// A point in a source file.
class position
{
- public:]m4_ifdef([b4_location_constructors], [[
+ public:
+ /// Type for line and column numbers.
+ typedef int counter_type;
+]m4_ifdef([b4_location_constructors], [[
/// Construct a position.
explicit position (]b4_percent_define_get([[filename_type]])[* f =
YY_NULLPTR,
- int l = ]b4_location_initial_line[,
- int c = ]b4_location_initial_column[)
+ counter_type l = ]b4_location_initial_line[,
+ counter_type c = ]b4_location_initial_column[)
: filename (f)
, line (l)
, column (c)
@@ -75,8 +78,8 @@ m4_define([b4_location_define],
]])[
/// Initialization.
void initialize (]b4_percent_define_get([[filename_type]])[* fn =
YY_NULLPTR,
- int l = ]b4_location_initial_line[,
- int c = ]b4_location_initial_column[)
+ counter_type l = ]b4_location_initial_line[,
+ counter_type c = ]b4_location_initial_column[)
{
filename = fn;
line = l;
@@ -86,7 +89,7 @@ m4_define([b4_location_define],
/** \name Line and Column related manipulators
** \{ */
/// (line related) Advance to the COUNT next lines.
- void lines (int count = 1)
+ void lines (counter_type count = 1)
{
if (count)
{
@@ -96,7 +99,7 @@ m4_define([b4_location_define],
}
/// (column related) Advance to the COUNT next columns.
- void columns (int count = 1)
+ void columns (counter_type count = 1)
{
column = add_ (column, count, ]b4_location_initial_column[);
}
@@ -105,13 +108,13 @@ m4_define([b4_location_define],
/// File name to which this position refers.
]b4_percent_define_get([[filename_type]])[* filename;
/// Current line number.
- int line;
+ counter_type line;
/// Current column number.
- int column;
+ counter_type column;
private:
/// Compute max (min, lhs+rhs).
- static int add_ (int lhs, int rhs, int min)
+ static counter_type add_ (counter_type lhs, counter_type rhs,
counter_type min)
{
return lhs + rhs < min ? min : lhs + rhs;
}
@@ -119,7 +122,7 @@ m4_define([b4_location_define],
/// Add \a width columns, in place.
inline position&
- operator+= (position& res, int width)
+ operator+= (position& res, position::counter_type width)
{
res.columns (width);
return res;
@@ -127,21 +130,21 @@ m4_define([b4_location_define],
/// Add \a width columns.
inline position
- operator+ (position res, int width)
+ operator+ (position res, position::counter_type width)
{
return res += width;
}
/// Subtract \a width columns, in place.
inline position&
- operator-= (position& res, int width)
+ operator-= (position& res, position::counter_type width)
{
return res += -width;
}
/// Subtract \a width columns.
inline position
- operator- (position res, int width)
+ operator- (position res, position::counter_type width)
{
return res -= width;
}
@@ -181,6 +184,8 @@ m4_define([b4_location_define],
class location
{
public:
+ /// Type for line and column numbers.
+ typedef position::counter_type counter_type;
]m4_ifdef([b4_location_constructors], [
/// Construct a location from \a b to \a e.
location (const position& b, const position& e)
@@ -196,8 +201,8 @@ m4_define([b4_location_define],
/// Construct a 0-width location in \a f, \a l, \a c.
explicit location (]b4_percent_define_get([[filename_type]])[* f,
- int l = ]b4_location_initial_line[,
- int c = ]b4_location_initial_column[)
+ counter_type l = ]b4_location_initial_line[,
+ counter_type c = ]b4_location_initial_column[)
: begin (f, l, c)
, end (f, l, c)
{}
@@ -205,8 +210,8 @@ m4_define([b4_location_define],
])[
/// Initialization.
void initialize (]b4_percent_define_get([[filename_type]])[* f =
YY_NULLPTR,
- int l = ]b4_location_initial_line[,
- int c = ]b4_location_initial_column[)
+ counter_type l = ]b4_location_initial_line[,
+ counter_type c = ]b4_location_initial_column[)
{
begin.initialize (f, l, c);
end = begin;
@@ -222,13 +227,13 @@ m4_define([b4_location_define],
}
/// Extend the current location to the COUNT next columns.
- void columns (int count = 1)
+ void columns (counter_type count = 1)
{
end += count;
}
/// Extend the current location to the COUNT next lines.
- void lines (int count = 1)
+ void lines (counter_type count = 1)
{
end.lines (count);
}
@@ -243,39 +248,45 @@ m4_define([b4_location_define],
};
/// Join two locations, in place.
- inline location& operator+= (location& res, const location& end)
+ inline location&
+ operator+= (location& res, const location& end)
{
res.end = end.end;
return res;
}
/// Join two locations.
- inline location operator+ (location res, const location& end)
+ inline location
+ operator+ (location res, const location& end)
{
return res += end;
}
/// Add \a width columns to the end position, in place.
- inline location& operator+= (location& res, int width)
+ inline location&
+ operator+= (location& res, location::counter_type width)
{
res.columns (width);
return res;
}
/// Add \a width columns to the end position.
- inline location operator+ (location res, int width)
+ inline location
+ operator+ (location res, location::counter_type width)
{
return res += width;
}
/// Subtract \a width columns to the end position, in place.
- inline location& operator-= (location& res, int width)
+ inline location&
+ operator-= (location& res, location::counter_type width)
{
return res += -width;
}
/// Subtract \a width columns to the end position.
- inline location operator- (location res, int width)
+ inline location
+ operator- (location res, location::counter_type width)
{
return res -= width;
}
@@ -304,7 +315,8 @@ m4_define([b4_location_define],
std::basic_ostream<YYChar>&
operator<< (std::basic_ostream<YYChar>& ostr, const location& loc)
{
- int end_col = 0 < loc.end.column ? loc.end.column - 1 : 0;
+ location::counter_type end_col
+ = 0 < loc.end.column ? loc.end.column - 1 : 0;
ostr << loc.begin;
if (loc.end.filename
&& (!loc.begin.filename
diff --git a/doc/bison.texi b/doc/bison.texi
index 0f5838c2..fbd3edfb 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -11453,8 +11453,8 @@ is some time and/or some talented C++ hacker
willing to contribute to Bison.
@node C++ Location Values
@subsection C++ Location Values
@c - %locations
-@c - class Position
-@c - class Location
+@c - class position
+@c - class location
@c - %define filename_type "const symbol::Symbol"
When the directive @code{%locations} is used, the C++ parser supports
@@ -11462,9 +11462,9 @@ location tracking, see @ref{Tracking Locations}.
By default, two auxiliary classes define a @code{position}, a single point
in a file, and a @code{location}, a range composed of a pair of
-@code{position}s (possibly spanning several files). But if the
-@code{%define} variable @code{api.location.type} is defined, then these
-classes will not be generated, and the user defined type will be used.
+@code{position}s (possibly spanning several files). If the @code{%define}
+variable @code{api.location.type} is defined, then these classes will not
be
+generated, and the user defined type will be used.
@menu
* C++ position:: One point in the source file
@@ -11477,13 +11477,17 @@ classes will not be generated, and the user
defined type will be used.
@node C++ position
@subsubsection C++ @code{position}
-@deftypeop {Constructor} {position} {} position (std::string* @var{file} =
nullptr, int @var{line} = 1, int @var{col} = 1)
+@defcv {Type} {position} {counter_type}
+The type used to store line and column numbers. Defined as @code{int}.
+@end defcv
+
+@deftypeop {Constructor} {position} {} position (std::string* @var{file} =
nullptr, counter_type @var{line} = 1, counter_type @var{col} = 1)
Create a @code{position} denoting a given point. Note that @code{file} is
not reclaimed when the @code{position} is destroyed: memory managed must be
handled elsewhere.
@end deftypeop
-@deftypemethod {position} {void} initialize (std::string* @var{file} =
nullptr, int @var{line} = 1, int @var{col} = 1)
+@deftypemethod {position} {void} initialize (std::string* @var{file} =
nullptr, counter_type @var{line} = 1, counter_type @var{col} = 1)
Reset the position to the given values.
@end deftypemethod
@@ -11494,28 +11498,28 @@ change it to @samp{@var{type}*} using
@samp{%define filename_type
"@var{type}"}.
@end deftypeivar
-@deftypeivar {position} {int} line
+@deftypeivar {position} {counter_type} line
The line, starting at 1.
@end deftypeivar
-@deftypemethod {position} {void} lines (int @var{height} = 1)
+@deftypemethod {position} {void} lines (counter_type @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} {int} column
+@deftypeivar {position} {counter_type} column
The column, starting at 1.
@end deftypeivar
-@deftypemethod {position} {void} columns (int @var{width} = 1)
+@deftypemethod {position} {void} columns (counter_type @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})
-@deftypemethodx {position} {position} operator+ (int @var{width})
-@deftypemethodx {position} {position&} operator-= (int @var{width})
-@deftypemethodx {position} {position} operator- (int @var{width})
+@deftypemethod {position} {position&} operator+= (counter_type @var{width})
+@deftypemethodx {position} {position} operator+ (counter_type @var{width})
+@deftypemethodx {position} {position&} operator-= (counter_type
@var{width})
+@deftypemethodx {position} {position} operator- (counter_type @var{width})
Various forms of syntactic sugar for @code{columns}.
@end deftypemethod
@@ -11538,11 +11542,11 @@ Create a @code{Location} from the endpoints of
the range.
@end deftypeop
@deftypeop {Constructor} {location} {} location (const position& @var{pos}
= position())
-@deftypeopx {Constructor} {location} {} location (std::string* @var{file},
int @var{line}, int @var{col})
+@deftypeopx {Constructor} {location} {} location (std::string* @var{file},
counter_type @var{line}, counter_type @var{col})
Create a @code{Location} denoting an empty range located at a given point.
@end deftypeop
-@deftypemethod {location} {void} initialize (std::string* @var{file} =
nullptr, int @var{line} = 1, int @var{col} = 1)
+@deftypemethod {location} {void} initialize (std::string* @var{file} =
nullptr, counter_type @var{line} = 1, counter_type @var{col} = 1)
Reset the location to an empty range at the given values.
@end deftypemethod
@@ -11551,15 +11555,15 @@ Reset the location to an empty range at the given
values.
The first, inclusive, position of the range, and the first beyond.
@end deftypeivar
-@deftypemethod {location} {void} columns (int @var{width} = 1)
-@deftypemethodx {location} {void} lines (int @var{height} = 1)
+@deftypemethod {location} {void} columns (counter_type @var{width} = 1)
+@deftypemethodx {location} {void} lines (counter_type @var{height} = 1)
Forwarded to the @code{end} position.
@end deftypemethod
-@deftypemethod {location} {location} operator+ (int @var{width})
-@deftypemethodx {location} {location} operator+= (int @var{width})
-@deftypemethodx {location} {location} operator- (int @var{width})
-@deftypemethodx {location} {location} operator-= (int @var{width})
+@deftypemethod {location} {location} operator+ (counter_type @var{width})
+@deftypemethodx {location} {location} operator+= (counter_type @var{width})
+@deftypemethodx {location} {location} operator- (counter_type @var{width})
+@deftypemethodx {location} {location} operator-= (counter_type @var{width})
Various forms of syntactic sugar for @code{columns}.
@end deftypemethod
