Re: filename() field should be const std::string* not std::string*

[Martin Blais]

On Mon, May 25, 2020 at 2:16 AM Akim Demaille <address@hidden> wrote:

> Hi Martin,
>
> > Le 24 mai 2020 à 16:56, Martin Blais <address@hidden> a écrit :
> >
> > Dear grammar-generators,
> > I'm setting up my %initial-action directive this morning (so much fun)
>
> I sense some true joy here :)  If you see how to improve things, don't
> hesitate making suggestions.
>

I have a ton of ideas, but the problem is that this project has too many
code generation options.
I don't have a clear map of how they all combine and fear it would be
brittle to change too much.


> and
> > I can't help notice that the C++ location() and position() constructors
> and
> > initializers accept an "std::string*" and not a "const std::string*",
> > despite not modifying their value anywhere. The docs on the "position"
> > class say:
> >
> > Instance Variable of position: std::string* file
>
> This should be filename.
>

Yep. Bison's docs need to be adjusted, I didn't misquote this:
https://www.gnu.org/software/bison/manual/html_node/C_002b_002b-position.html



> >  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 ‘type*’ using ‘%define filename_type "type"’.
> >
> > The generated parser doesn't seem to modify it.
> > This forces by driver object to cast away what should otherwise be a
> > comforting const access.
>
> What do you mean?  Do you have an example of this?  In my parsers, I don't
> need it.
>

My scanner (re/flex) didn't fill in the filename field so I was trying to
do that in the parser in an %initial-action block.
That block called a const accessor on the driver (e.g. const std::string*
get_filename() const), but it couldn't since storing that into std::string*
will generate an error.

(It turns out it doesn't work anyway (the filename gets set on the
lookahead token in that block), so instead I overrode the method on the
scanner that produces the location to do that directly.)



>
> > I can "fix" this by using
> > %define filename_type "const std::string"
> >
> > Is there any reason not to make the default const?
>
> Wow, it dates back to the creation of lalr1.cc.  See
>
> commit 7548fed236e50c5ba9933c373a537f1a1dd15224
> Author: Akim Demaille <address@hidden>
> Date:   Thu Feb 6 10:04:29 2003 +0000
>
> and then 9a0d8becd844eba80ff657a7391b2e85b7ff2077 for the introduction
> of filename_type.
>
> I agree the parser doesn't need to be able to change the file name.
> However that does not suffice to make it const: the user might have
> reasons to change the file name.
>

If the position/location itself doesn't change it it should hold the
pointer as const. Unless you want to support that the user write some rule
processing code that accesses the shared filename string object via
position/location and mutates it in place, which seems to go counter to the
spirit of how this is intended to be (and the side-effect of which comes
close to causing me and other functional programmers reading this to have a
stroke).


I'm not sure why she would though.


> We could indeed try to make that change and see what happens.  The
> test suite is happy, but will the users be?
>
>
> Cheers!