[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: C++: Pointer to non-const std::string in position and location shoul
|
From: |
Akim Demaille |
|
Subject: |
Re: C++: Pointer to non-const std::string in position and location should be a pointer to const std::string |
|
Date: |
Sat, 27 Jun 2020 10:08:05 +0200 |
Hi Yuriy, hi Martin,
We accept visitors :)
> Le 26 juin 2020 à 00:22, Yuriy Solodkyy <solodon@gmail.com> a écrit :
>
> Hi,
>
> I’m using bison 3.6.3 on MacOS via brew. I see that in the C++ generated
> parser location and position take pointer to std::string with filename via a
> pointer to modifiable std::string. I don’t see the filename itself being
> modified anywhere in the parser, so I was wondering why not accept a pointer
> to const std::string instead? Without this, I need to either copy filename
> first somewhere or const_cast when I can ensure the lifetime. The thing is
> that when I invoke my own function passing it a filename in const string&,
> which in turn calls parse (in pull mode) I can ensure that the filename will
> be valid during the entire duration of the call to parse yet I cannot use a
> pointer to it to initialize location because location demands the pointer to
> non-const string, which in my opinion is unnecessary. Can you please change
> your source for position and location take pointer to const std::string
> instead?
Martin asked the same thing very recently:
https://lists.gnu.org/r/help-bison/2020-05/msg00011.html
Ok, let's do it. I hope it won't break too many things (it's quite easy for
people who relied on the previous default to add `%define filename_type
"std::string"` is get a consistent behavior before/after this change).
WDYT about the following patch?
Cheers!
commit eeafc706e87dab7e84d0056c852e4579e6c3cf53
Author: Akim Demaille <akim.demaille@gmail.com>
Date: Sat Jun 27 09:43:14 2020 +0200
c++: by default, use const std::string for file names
Reported by Martin Blais and Yuriy Solodkyy.
https://lists.gnu.org/r/help-bison/2020-05/msg00011.html
https://lists.gnu.org/r/bug-bison/2020-06/msg00038.html
While at it, modernize filename_type as api.filename.type and document
it properly.
* data/skeletons/c++.m4 (filename_type): Rename as...
(api.filename.type): this.
Default to const std::string.
* data/skeletons/location.cc (position, location): Expose the
filename_type type.
Use api.filename.type.
* doc/bison.texi (%define Summary): Document api.filename.type.
(C++ Location Values): Document position::filename_type.
* src/muscle-tab.c (muscle_percent_variable_update): Ensure backward
compatibility.
* tests/c++.at: Check that using const file names is ok.
tests/input.at: Check backward compat.
diff --git a/NEWS b/NEWS
index d9da8192..29425bca 100644
--- a/NEWS
+++ b/NEWS
@@ -2,11 +2,38 @@ GNU Bison NEWS
* Noteworthy changes in release ?.? (????-??-??) [?]
+** New features
+
+*** File prefix mapping
+
+ Bison learned a new argument, '--file-prefix-map OLD=NEW'. Any file path in
+ the output (specifically #line directives and #ifdef header guards) that
+ being with the prefix OLD will have it replace with the prefix NEW, similar
+ to the -ffile-prefix-map in GCC. This option can be used to make bison output
+ reproducible.
+
** Changes
- When installed to be relocatable (via configure --enable-relocatable),
+*** Relocatable installation
+
+ When installed to be relocatable (via `configure --enable-relocatable`),
bison will now also look for a relocated m4.
+*** C++ file names
+
+ The `filename_type` %define variable was renamed `api.filename.type`.
+ Instead of
+
+ %define filename_type "symbol"
+
+ write
+
+ %define api.filename.type {symbol}
+
+ (Or let `bison --update` do it for you).
+
+ It now defaults to `const std::string` instead of `std::string`.
+
** Bug fixes
*** Include the generated header (yacc.c)
@@ -48,16 +75,6 @@ GNU Bison NEWS
An old, well hidden, bug in the generation of IELR parsers was fixed.
-** New features
-
-*** File prefix mapping
-
- Bison learned a new argument, '--file-prefix-map OLD=NEW'. Any file path in
- the output (specifically #line directives and #ifdef header guards) that
- being with the prefix OLD will have it replace with the prefix NEW, similar
- to the -ffile-prefix-map in GCC. This option can be used to make bison output
- reproducible.
-
* Noteworthy changes in release 3.6.4 (2020-06-15) [stable]
** Bug fixes
diff --git a/THANKS b/THANKS
index af24ceaa..e9bc2762 100644
--- a/THANKS
+++ b/THANKS
@@ -112,6 +112,7 @@ Marc Autret autret_m@epita.fr
Marc Mendiola mmendiol@usc.edu
Marc Schönefeld marc.schoenefeld@gmx.org
Mark Boyall wolfeinstein@gmail.com
+Martin Blais blais@furius.ca
Martin Jacobs martin.jacobs@arcor.de
Martin Mokrejs mmokrejs@natur.cuni.cz
Martin Nylin martin.nylin@linuxmail.org
@@ -215,6 +216,7 @@ Wolfram Wagner ww@mpi-sb.mpg.de
Wwp subscript@free.fr
xolodho xolodho@gmail.com
Yuichiro Kaneko spiketeika@gmail.com
+Yuriy Solodkyy solodon@gmail.com
Zack Weinberg zack@codesourcery.com
江 祖铭 jjzuming@outlook.com
長田偉伸 cbh34680@iret.co.jp
diff --git a/data/skeletons/c++.m4 b/data/skeletons/c++.m4
index 1d41c4c8..5536b1a0 100644
--- a/data/skeletons/c++.m4
+++ b/data/skeletons/c++.m4
@@ -105,7 +105,7 @@ b4_percent_define_default([[api.parser.class]], [[parser]])
#
# b4_percent_define_default([[api.location.type]], [[location]])
-b4_percent_define_default([[filename_type]], [[std::string]])
+b4_percent_define_default([[api.filename.type]], [[const std::string]])
# Make it a warning for those who used betas of Bison 3.0.
b4_percent_define_default([[api.namespace]], m4_defn([b4_prefix]))
diff --git a/data/skeletons/location.cc b/data/skeletons/location.cc
index dff984e7..33c9e50d 100644
--- a/data/skeletons/location.cc
+++ b/data/skeletons/location.cc
@@ -63,11 +63,13 @@ m4_define([b4_location_define],
class position
{
public:
+ /// Type for file name.
+ typedef ]b4_percent_define_get([[api.filename.type]])[ filename_type;
/// 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,
+ explicit position (filename_type* f = YY_NULLPTR,
counter_type l = ]b4_location_initial_line[,
counter_type c = ]b4_location_initial_column[)
: filename (f)
@@ -77,7 +79,7 @@ m4_define([b4_location_define],
]])[
/// Initialization.
- void initialize (]b4_percent_define_get([[filename_type]])[* fn =
YY_NULLPTR,
+ void initialize (filename_type* fn = YY_NULLPTR,
counter_type l = ]b4_location_initial_line[,
counter_type c = ]b4_location_initial_column[)
{
@@ -106,7 +108,7 @@ m4_define([b4_location_define],
/** \} */
/// File name to which this position refers.
- ]b4_percent_define_get([[filename_type]])[* filename;
+ filename_type* filename;
/// Current line number.
counter_type line;
/// Current column number.
@@ -184,6 +186,8 @@ m4_define([b4_location_define],
class location
{
public:
+ /// Type for file name.
+ typedef position::filename_type filename_type;
/// Type for line and column numbers.
typedef position::counter_type counter_type;
]m4_ifdef([b4_location_constructors], [
@@ -200,7 +204,7 @@ 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,
+ explicit location (filename_type* f,
counter_type l = ]b4_location_initial_line[,
counter_type c = ]b4_location_initial_column[)
: begin (f, l, c)
@@ -209,7 +213,7 @@ m4_define([b4_location_define],
])[
/// Initialization.
- void initialize (]b4_percent_define_get([[filename_type]])[* f =
YY_NULLPTR,
+ void initialize (filename_type* f = YY_NULLPTR,
counter_type l = ]b4_location_initial_line[,
counter_type c = ]b4_location_initial_column[)
{
diff --git a/doc/bison.texi b/doc/bison.texi
index 5a81202b..ac71fe6d 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -5922,6 +5922,31 @@ Unaccepted @var{variable}s produce an error. Some of
the accepted
@var{variable}s are described below.
+@c ================================================== api.filename.file
+@anchor{api-filename-type}
+@deffn {Directive} {%define api.filename.type} @{@var{type}@}
+
+@itemize @bullet
+@item Language(s): C++
+
+@item Purpose:
+Define the type of file names in Bison's default location and position
+types. @xref{Exposing the Location Classes}.
+
+@item Accepted Values:
+Any type that is printable (via streams) and comparable (with @code{==} and
+@code{!=}).
+
+@item Default Value: @code{const std::string}.
+
+@item History:
+Introduced in Bison 2.0 as @code{filename_type} (with @code{std::string} as
+default), renamed as @code{api.filename.type} in Bison 3.7 (with @code{const
+std::string} as default).
+@end itemize
+@end deffn
+
+
@c ================================================== api.header.include
@deffn Directive {%define api.header.include} @{"header.h"@}
@deffnx Directive {%define api.header.include} @{<header.h>@}
@@ -6052,7 +6077,8 @@ Introduced in Bison 3.2.
@item Default Value: none
@item History:
-Introduced in Bison 2.7 for C++ and Java, in Bison 3.4 for C.
+Introduced in Bison 2.7 for C++ and Java, in Bison 3.4 for C. Was
+originally named @code{location_type} in Bison 2.5 and 2.6.
@end itemize
@end deffn
@@ -6555,12 +6581,6 @@ Introduced in Bison 3.0.3.
@c api.value.type
-@c ================================================== location_type
-@deffn Directive {%define location_type}
-Obsoleted by @code{api.location.type} since Bison 2.7.
-@end deffn
-
-
@c ================================================== lr.default-reduction
@deffn Directive {%define lr.default-reduction} @var{when}
@@ -11898,10 +11918,6 @@ 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 - %define filename_type "const symbol::Symbol"
When the directive @code{%locations} is used, the C++ parser supports
location tracking, see @ref{Tracking Locations}.
@@ -11923,25 +11939,28 @@ generated, and the user defined type will be used.
@node C++ position
@subsubsection C++ @code{position}
+@defcv {Type} {position} {filename_type}
+The base type for file names. Defaults to @code{const std::string}.
+@xref{api-filename-type,,@code{api.filename.type}}, to change its definition.
+@end defcv
+
@defcv {Type} {position} {counter_type}
The type used to store line and column numbers. Defined as @code{int}.
@end defcv
-@deftypeop {Constructor} {position} {} position (@code{std::string*}
@var{file} = nullptr, @code{counter_type} @var{line} = 1, @code{counter_type}
@var{col} = 1)
+@deftypeop {Constructor} {position} {} position (@code{filename_type*}
@var{file} = nullptr, @code{counter_type} @var{line} = 1, @code{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 (@code{std::string*} @var{file} =
nullptr, @code{counter_type} @var{line} = 1, @code{counter_type} @var{col} = 1)
+@deftypemethod {position} {void} initialize (@code{filename_type*} @var{file}
= nullptr, @code{counter_type} @var{line} = 1, @code{counter_type} @var{col} =
1)
Reset the position to the given values.
@end deftypemethod
-@deftypeivar {position} {std::string*} file
+@deftypeivar {position} {filename_type*} file
The name of the file. It will always be handled as a pointer, the parser
-will never duplicate nor deallocate it. As an experimental feature you may
-change it to @samp{@var{type}*} using @samp{%define filename_type
-"@var{type}"}.
+will never duplicate nor deallocate it.
@end deftypeivar
@deftypeivar {position} {counter_type} line
@@ -11988,11 +12007,11 @@ Create a @code{Location} from the endpoints of the
range.
@end deftypeop
@deftypeop {Constructor} {location} {} location (@code{const position&}
@var{pos} = position())
-@deftypeopx {Constructor} {location} {} location (@code{std::string*}
@var{file}, @code{counter_type} @var{line}, @code{counter_type} @var{col})
+@deftypeopx {Constructor} {location} {} location (@code{filename_type*}
@var{file}, @code{counter_type} @var{line}, @code{counter_type} @var{col})
Create a @code{Location} denoting an empty range located at a given point.
@end deftypeop
-@deftypemethod {location} {void} initialize (@code{std::string*} @var{file} =
nullptr, @code{counter_type} @var{line} = 1, @code{counter_type} @var{col} = 1)
+@deftypemethod {location} {void} initialize (@code{filename_type*} @var{file}
= nullptr, @code{counter_type} @var{line} = 1, @code{counter_type} @var{col} =
1)
Reset the location to an empty range at the given values.
@end deftypemethod
diff --git a/src/muscle-tab.c b/src/muscle-tab.c
index 096c93ca..2cc136a1 100644
--- a/src/muscle-tab.c
+++ b/src/muscle-tab.c
@@ -447,6 +447,7 @@ muscle_percent_variable_update (char const *variable,
{ "api.push_pull", "api.push-pull",
muscle_keyword },
{ "api.tokens.prefix", "api.token.prefix", muscle_code },
{ "extends", "api.parser.extends",
muscle_keyword },
+ { "filename_type", "api.filename.type", muscle_code },
{ "final", "api.parser.final",
muscle_keyword },
{ "implements", "api.parser.implements",
muscle_keyword },
{ "lex_symbol", "api.token.constructor", -1 },
diff --git a/tests/c++.at b/tests/c++.at
index adc41f31..5cbbc91e 100644
--- a/tests/c++.at
+++ b/tests/c++.at
@@ -46,38 +46,45 @@ template <typename T>
bool
check (const T& in, const std::string& s)
{
+ const static bool verbose = getenv ("VERBOSE");
std::stringstream os;
os << in;
- if (os.str () != s)
+ if (os.str () == s)
+ {
+ if (verbose)
+ std::cerr << os.str () << '\n';
+ return true;
+ }
+ else
{
std::cerr << "fail: " << os.str () << ", expected: " << s << '\n';
return false;
}
- return true;
}
int
main (void)
{
+ const std::string fn = "foo.txt";
int fail = 0;
- ]AT_YYLTYPE[ loc; fail += check (loc, "1.1");
- fail += check (loc + 10, "1.1-10");
- loc += 10; fail += check (loc, "1.1-10");
- loc += -5; fail += check (loc, "1.1-5");
- fail += check (loc - 5, "1.1");
- loc -= 5; fail += check (loc, "1.1");
+ ]AT_YYLTYPE[ loc (&fn); fail += check (loc, "foo.txt:1.1");
+ fail += check (loc + 10, "foo.txt:1.1-10");
+ loc += 10; fail += check (loc, "foo.txt:1.1-10");
+ loc += -5; fail += check (loc, "foo.txt:1.1-5");
+ fail += check (loc - 5, "foo.txt:1.1");
+ loc -= 5; fail += check (loc, "foo.txt: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 -= 10; fail += check (loc, "foo.txt: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");
+ loc.columns (10); loc.lines (10); fail += check (loc, "foo.txt:1.1-11.0");
+ loc.lines (-2); fail += check (loc, "foo.txt:1.1-9.0");
+ loc.lines (-10); fail += check (loc, "foo.txt:1.1");
]AT_YYLTYPE[ loc2 (YY_NULLPTR, 5, 10);
fail += check (loc2, "5.10");
- fail += check (loc + loc2, "1.1-5.9");
- loc += loc2; fail += check (loc, "1.1-5.9");
+ fail += check (loc + loc2, "foo.txt:1.1-5.9");
+ loc += loc2; fail += check (loc, "foo.txt:1.1-5.9");
return !fail;
}
]])
diff --git a/tests/input.at b/tests/input.at
index d152de31..f7afee2e 100644
--- a/tests/input.at
+++ b/tests/input.at
@@ -2220,6 +2220,7 @@ AT_DATA([[input.y]],
%define namespace "foo"
%define variant
%define parser_class_name {parser}
+%define filename_type {filename}
%%
start: %empty;
]])
@@ -2244,6 +2245,10 @@ input.y:5.1-34: warning: deprecated directive: '%define
parser_class_name {parse
5 | %define parser_class_name {parser}
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| %define api.parser.class {parser}
+input.y:6.1-32: warning: deprecated directive: '%define filename_type
{filename}', use '%define api.filename.type {filename}' [-Wdeprecated]
+ 6 | %define filename_type {filename}
+ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ | %define api.filename.type {filename}
input.y:2.1-40: error: invalid value for %define Boolean variable
'lr.keep-unreachable-state'
2 | %define lr.keep_unreachable_states maybe
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: C++: Pointer to non-const std::string in position and location should be a pointer to const std::string, Jacob L. Mandelson, 2020/06/27