[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: |
Yuriy Solodkyy |
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 01:33:40 -0700 |
Hi Akim!
Thanks for the prompt reply and patch! Somehow I missed %define filename_type
in your documentation, otherwise I would have jumped on that. It does make
sense to have const to be the default though, so the change would be great!
Speaking of Bison documentation, I was wondering if there is a way to generate
a C++ push parser? It seems like that is not the case at the moment, although I
don’t see why given that all the machinery already exists for C. Can you
confirm this please?
Thank you!
Yuriy
> On Jun 27, 2020, at 01:08, Akim Demaille <akim@lrde.epita.fr> wrote:
>
> 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