Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add %define api.value.type feature, fixes #6 #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madopew
Copy link
Contributor

@madopew madopew commented Nov 4, 2020

Thanks for the extension, it’s great.

So here’s my attempt to fix #6. I’ve tried my best to remain the code style as it was but probably missed some meaning (or forgot about some details). I’ve added new parser states and modified regex for word (it now includes minus sign). Some details:

  1. %define variables are defined in switch statement, so possibility to add new features still remains. %defined data type saved as default;
  2. %union highlighted as warning if api.value.type is defined (and visa versa);
  3. if non terminal is not defined with %type and default type is defined, non terminal’s type set to default

@babyraging
Copy link
Owner

Hi, thank you for the PR.

I've reviewed your code, it looks good.
However, I think that if we want to support the %define option, we need to keep in mind all other possible define values, that are defined in bison.
https://www.gnu.org/software/bison/manual/html_node/_0025define-Summary.html

The solution you proposed won't be very extensible for other %define values, although we might not need to handle the others...... as they are rarely used... XD

Some problems I've found in your code
https://github.com/babyraging/yash/pull/8/files#diff-2fb9a0bec2bff7c8d6f1dbb6a530c02908493f34e4ba23d0afe072a10b8a47cfR218-R223
Actually, this can be allowed if the defined type is union-directive

https://github.com/babyraging/yash/pull/8/files#diff-8520701a43509ebb5bdceb328ede5beb98e91121807c434dbd6ff112f8039a04R15
Here I think we cannot have the minus sign in a word. A word supposes to be a token o a non-terminal, I think you cannot declare a token with the minus sign, can you?

@babyraging
Copy link
Owner

We might need to support this too

%code requires
{
  struct my_value
  {
    enum
    {
      is_int, is_str
    } kind;
    union
    {
      int ival;
      char *sval;
    } u;
  };
}
%define api.value.type {struct my_value}
%token <u.ival> INT "integer"
%token <u.sval> STR "string"

@madopew
Copy link
Contributor Author

madopew commented Nov 5, 2020

Hi, thanks for reply!

While trying to fix that issue I already realized how actually problematic it is to add %define :)
Here are some points:

Actually, this can be allowed if the defined type is union-directive

Well yes, it says that in the manual but I've actually tried to run it and Bison threw me an error:

%define api.value.type union-directive

%token TOKEN

%union {
    char *;
}

%%

expr
    : TOKEN
    ;

%%
$ bison test.y
test.y:1.9-22: error: '%union' and '%define api.value.type' cannot be used together
 %define api.value.type union-directive
         ^^^^^^^^^^^^^^

(worth to mention, it's not even a warning, it's an error)

A word supposes to be a token o a non-terminal, I think you cannot declare a token with the minus sign, can you?

You are right, missed that. For some reason I thought C id's support the minus sign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Type is not declared" on %define api.value.type
2 participants