Re: [PATCH] Style change and some factoring

[Akim Demaille]

Le 4 juil. 09 à 20:50, Alex Rozenman a écrit :

> Hi

Hi Alex,

Thanks for your changes!

> On Sun, Jun 28, 2009 at 2:57 PM, Akim Demaille <address@hidden>  
> wrote:
> > Too eager to play with your patch, I made several errors in my  
> > grammar.  I had a $from.name_get() but I forgot to declare [from].   
> > Bison answered:
> > ugrammar-pruned.y:534.37-50: reference is invalid: ` 
> > $from.name_get', symbol not found
> > IMHO it should refer to $from, not to $from.name_get.
> I will change it. I can agree that in case of "symbol not found"  
> error we should show shortest possible symbol (without dots and  
> dashes).

I don't remember what decision we made.  $foo.bar is always an  
_error_, or it's a warning?  We have -Werror, so I prefer warnings,  
but I'll stick to whatever consensus we reached.


> > Also, I had a $9foo, where 9 should have been removed.  It accepted  
> > $9 and left the "foo" attached to the expansion of $9.  Now that we  
> > have $[] to make what we mean clear, I think that it should  
> > complain about $9foo, like it does for the grammar symbol 9foo.  We  
> > should support $[9]foo for those who really mean that.
> > This applies to other values of 9 and foo :)
> I would agree with that, except it will affect backward  
> compatibility. I tried the same example with an old version of bison  
> and got exactly the same behaviour: only digit was eaten and all the  
> rest was attached in the generated code.

I see no valid case where this "feature" could be used.  Now, we will  
even be able to mean $[1]2, which was not possible before.


> > More doc please.  What is "long int"?  Probably the domain for ind,  
> > so please introduce some "variant_index" typedef.
> Please note that, actually there is two domains for indexes. When we  
> speak about named references, "unsigned" is very enough (I changed  
> it). But for positional references, there is an option for $-3  
> "deep" stack references. This is the reason that "parse_ref"  
> function must return a signed value. "long int" is also taken from  
> old code with intention to keep existing functionality.


There is a bazillion of diverse uses of integral types in Bison at  
many different places, and years ago "int" ruled bisonland.   
Introducing typedef helped a lot to make the code clearer, and I'd  
like to see "int" stay away.  So even if it means two typedefs, I'd  
prefer that.

Here are a few more comments on your changes.  Your work is brilliant,  
and it's going to be a really well-received feature when Bison is  
released.  Please, don't forget to forge a nice NEWS entry.  And  
then... the documentation :)


> diff --git a/src/scan-code.l b/src/scan-code.l
> index 28a9fe4..78cf839 100644
> --- a/src/scan-code.l
> +++ b/src/scan-code.l
> @@ -181,7 +184,7 @@ ref      -?[0-9]+|{id}|"["{id}"]"|"$"
>    "$"("<"{tag}">")?{ref}  {
>      ref_tail_fields = 0;
>      handle_action_dollar (self->rule, yytext, *loc);
> -    if (ref_tail_fields != NULL) {
> +    if (ref_tail_fields) {
>        obstack_sgrow (&obstack_for_string, ref_tail_fields);
>      }

Useless braces.  And braces should be alone on their line.

>      need_semicolon = true;
> @@ -189,7 +192,7 @@ ref      -?[0-9]+|{id}|"["{id}"]"|"$"
>    "@"{ref} {
>      ref_tail_fields = 0;
>      handle_action_at (self->rule, yytext, *loc);
> -    if (ref_tail_fields != NULL) {
> +    if (ref_tail_fields) {
>        obstack_sgrow (&obstack_for_string, ref_tail_fields);
>      }

Likewise.


> +/* Set when the variant refers to a symbol hidden
> +   by an explicit symbol reference. */
> +#define VARIANT_HIDDEN (1 << 0)
> +
> +/* Set when the variant refers to a symbol containing
> +   dots or dashes. Will require explicit bracketing. */
> +#define VARIANT_BAD_BRACKETING (1 << 1)
> +
> +/* Set when the variant refers to a symbol which is
> +   not visible from current midrule. */
> +#define VARIANT_NOT_VISIBLE_FROM_MIDRULE (1 << 2)

I forgot to mention this the first time: could you please make this an
enum?  Enums are scoped, they are visible from debuggers and so forth.


> @@ -399,7 +399,7 @@ splice  (\\[ \f\t\v]*\n)*
>      }
>        }
>      else
> -      complain_at (*loc, _("a non empty identifier expected"));
> +      complain_at (*loc, _("an identifier expected"));

This reads slightly akward.  What about "expected an identifier"?


> @@ -452,6 +451,6 @@ AT_DATA_GRAMMAR([test.y],
>  start[a s]: foo
>  ]])
>  AT_BISON_CHECK([-o test.c test.y], 1, [],
> -[[test.y:11.9: redundant identifier in bracketed name: `s'
> +[[test.y:11.9: unexpected identifier in bracketed name: `s'

We should check, but I'm not sure we use `quotes' when the culprit is
the last guy on the line.

    unexpect `foo'
    unexpected identifier: foo

Don't pay too much attention to this, first Bison should have a
guideline for messages.