[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Style change and some factoring

From: Akim Demaille
Subject: Re: [PATCH] Style change and some factoring
Date: Wed, 8 Jul 2009 12:56:40 +0200

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


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. $ 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);
>      }


> +/* 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. */

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]