Re: C++: Pointer to non-const std::string in position and location should be a pointer to const std::string

[Yuriy Solodkyy]

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
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>