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

Hi Yuriy, hi Martin,

We accept visitors :)

> Le 26 juin 2020 à 00:22, Yuriy Solodkyy <[email protected]> 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 <[email protected]>
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               [email protected]
 Marc Mendiola             [email protected]
 Marc Schönefeld           [email protected]
 Mark Boyall               [email protected]
+Martin Blais              [email protected]
 Martin Jacobs             [email protected]
 Martin Mokrejs            [email protected]
 Martin Nylin              [email protected]
@@ -215,6 +216,7 @@ Wolfram Wagner            [email protected]
 Wwp                       [email protected]
 xolodho                   [email protected]
 Yuichiro Kaneko           [email protected]
+Yuriy Solodkyy            [email protected]
 Zack Weinberg             [email protected]
 江 祖铭                    [email protected]
 長田偉伸                   [email protected]
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
       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~