Placement of Constant Grammar__Version = 2;

Now that I the 6.42 version of my Inform6 for Unix package out, I started a new development branch for tinkering with what will become Inform 6.43. When I built the package, I got complaints from the compiler when building two of the PunyInform test programs:

Inform 6.43 for Linux (in development)
"punyinform/lib/globals.h", line 87: Error:  Once an action has been defined it is too late to change the grammar version.
> Constant Grammar__Version = 2;
line 156: Error:  The 'topic' token is only available if you are using grammar version 2 or later
>  * topic
line 156: Error:  '/' can only be used with grammar version 2 or later
...

I then tracked this change to Hook up the GRAMMAR_VERSION option. · DavidKinder/Inform6@2f1bd4a · GitHub. What was the reasoning for this change?

My working theory on this is that the programmer should NOT declare any functions or do anything else with grammar prior to including parser.h. I haven’t tripped on anything in my own code yet, but I did submit changes for the PunyInform test code.

1 Like

The grammar version determines the grammar table format. Does it make sense to change that after you’ve started adding grammar table entries? What will the table wind up containing? “A broken mix of formats” is the obvious answer. It doesn’t seem worth combing through the code to prove otherwise.

My test suite includes library_of_horror.inf (with punylib 3.6, but I don’t think the layout has changed). This includes globals.h as its first include, before declaring anything but constants and abbreviations. This is the way it’s supposed to work.

My working theory on this is that the programmer should NOT declare any functions or do anything else with grammar prior to including parser.h .

Declaring functions is okay, but not usual. The normal pattern is to include parser.h as close to the top as possible. What you can’t do is use Verb or Fake_Action directives before that.

(Using Fake_Action before Grammar__Version has always been an error, because the results are blatantly broken – you get inconsistent fake-action values.)

1 Like

Here’s an example of the trouble:

The case that currently breaks, which David has submitted a PR for, is a function which uses action numbers, e.g.

switch(action) {
  Take: "You can't!";
}

This function is declared before including globals.h. Compiling this has worked fine until now. This particular function can just as easily be declared after including puny.h (which in turn includes grammar.h, parser.h etc), but it’s interesting to learn if there’s a particular reason why this doesn’t work anymore.

1 Like

Changing code at line 198++ in direct.c to (change error to warning and allow changing of GV):

            if (i == grammar_version_symbol) {
                /* Special case for changing Grammar__Version. We check
                   conditions carefully before applying the change. */
                if (AO.marker != 0) {
                    error("Grammar__Version must be given an explicit constant value");
                }
                else if (grammar_version_number == AO.value) {
                    /* no change needed */
                }
                else if (no_fake_actions > 0) {
                    warning("Once a fake action has been defined \
it is too late to change the grammar version.");
                    set_grammar_version(AO.value);
                }
                else if (no_actions > 0) {
                    warning("Once an action has been defined \
it is too late to change the grammar version.");
                    set_grammar_version(AO.value);
                }
                else {
                    set_grammar_version(AO.value);
                }
            }

Produces identical bytecode for test-parser.inf compiled with 6.42 and 6.43 in development.

Ok, here is a possible solution…

no_actions are increased in function action_of_name (verbs.c). This function is called from a couple of places but it is only first when grammar lines are created that it is too late to backpatch and allow a change to the grammar__version constant. This is done in the function grammar_line (verbs.c). Fortunately there is a counter that keeps track on how many grammar lines that have been built. So instead of checking on no_actions we could check on no_grammar_lines instead.

In short, change lines 211-214 in directs.c to something like:

                else if (no_grammar_lines > 0) {
                    error("Once a grammar has been defined \
it is too late to change the grammar version.");
                }

This will keep previous behaviour and produce identical bytecode but still report it as an error when we reached a point when it’s no longer possible to change version.

Thanks. I see the difficulty and yes, changing that test to no_grammar_lines > 0 is a good idea.

I’ll include that change in my next grammar PR. (I’m working on “Accept two Verb declarations for the same verb, with a warning · Issue #288 · DavidKinder/Inform6 · GitHub” this weekend, and I also want to get the GV3 feature in.)

1 Like

To explain further: the compiler creates action entries (no_actions) when you use a ##Action constant or switch on an action name. This doesn’t create grammar table entries (no_grammar_lines) yet, though. That only happens in the Verb and Extend directives.