help-bison
[Top][All Lists]
Advanced

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

Re: %merge confusion


From: Jot Dot
Subject: Re: %merge confusion
Date: Fri, 1 Jan 2021 15:32:11 -0700 (MST)

> Jot, as I have already stated several times, your merger is *typed*,
> which is an undocumented feature.  The signature of your merger,
> as mentioned several times in my messages, should not be
> 
> YYSTYPE stmtMerge(YYSTYPE x0, YYSTYPE x1)
> 
> but
> 
> gen::index_t stmtMerge(YYSTYPE x0, YYSTYPE x1)
> 
> The generated code is right, your merger is wrong.

That makes it clear!

FWIW: The only reference I found to this signature change was in *one* prior 
email.

In my last two emails, I even mention my prototype was still "YYSTYPE  .." and
only now you've corrected me on this.
> It is as what the manual requested:
> static YYSTYPE stmtMerge (YYSTYPE x0, YYSTYPE x1);
I hesitated to post my merge code since I figured it would confuse the issue.
Which it did. You were confused as to what I was doing inside the routine and
missed the signature problem.

Now what you mentioned in a previous email is now clear:

> The nature of the data (POD or object) is irrelevant here.  The question is
> really that currently there are two signatures for mergers:
> 
> untyped: (YYSTYPE, YYSTYPE) -> YYSTYPE
> typed:   (YYSTYPE, YYSTYPE) -> gen::index_t
> 
> The typed signature has two problems: (a) it is badly implemented in the
> case of api.value.type=union, and (b) I don't understand why the typed
> alternative is not
> 
> (gen::index_t, gen::index_t) -> gen::index_t

Looking at this now, I see where my prototype for the function changed.
I focused only on the input arguments and not on the return type.
Sorry, I did not pick up on it.

But, in my defence:
        *******************************************************
        *** That wasn't what I was asking for all this time ***
        *******************************************************

> The else-clause (which is the one that introduces the yy0->TYPE (the names 
> have
> changed since then) which is broken in your case) is undocumented ....

I've asked many times in different ways what was I was doing that makes
this a "typed merger" and how I could change it so I do not use this "typed 
merger"
signature and just use "untyped" merges. Basically:
> untyped: (YYSTYPE, YYSTYPE) -> YYSTYPE      // Should I be doing this 
> instead? It's what I anticipated anyways.
> typed:   (YYSTYPE, YYSTYPE) -> gen::index_t // I'm here but is broken in my 
> case?

Obviously I was not clear in my intents and apologise for the frustrations it 
caused.

So: Let's hit "reset" w/rt this topic and let me see if I am correctly
summarising everything:
1) api.value.type variant is being worked on. Don't use this path yet.
2) api.value.type union is what was broken, not "typed mergers"
3) %union works
4) "typed mergers" work but the merge prototype changes.
5) The "untyped merger" is when I don't use %token and %type but that is
   not applicable in my usage case so I must use "typed mergers"

So, using 3) and 4) *properly*, I should have no issues.

Please correct and/or add anything pertinent w/rt this issue.


re: %merge routine and arguments
>> The incoming arguments are ok but there is no parsing context provided.
>> It would be (to me) a quite useful extension if you added something like:
>> %merge-param { something }

> I'm not convinced you actually need this.  I feel your semantic values
> are too "weak".  I mean, I believe what you do need is to use richer
> types for semantic values, into which you can plug that additional
> information.

That does make lots of sense. But first please understand where I am coming 
from:

Look at from this viewpoint: Conceptually I feel the merge routines are really
an extension of the parser and not just some abstract user code that is run.

I wished Bison would spit out prototypes in the Parser class for each merge 
routine
in the form:

class Parser
{ ...
  ...
  YYSTYPE stmtMerge(YYSTYPE y0, YYSTYPE y1); // One for each different merge 
routine used
};

and we just supply the implementation.

Conceptually, this is how I envision merge routines *should* have been handled.
I would not need any additional parameter as I could access the %parse-param 
directly
whenever I need to. In my case, I only need it in my merge routine.
The %merge-param substitutes for this in a "backwards compatible way".

FWIW: I don't think you realised what I was doing with my merge routine.
I *am* returning an 'index' type. The struct was only used to "expand"
the union and thus "the relevant parts in that struct" was clear of the
"normal" part of the union. so This way I could *pass in* values.
Names should be changed to make it clearer.

Rewritten a bit better:
>> 
>> %union {
>>    gen::index_t index;
>>    struct parse_param_t {
>>        char filler[sizeof gen::index_t index]; // Set this to the size of 
>> the union excluding this struct
>>        // Following member(s) will never get written over by "normal union 
>> variable use"
>>        class MyTree * pResult;
>>    } parse_param;       // A struct to hold *** data passed in***
>> }

Then it *might* have made this more clear if I wrote:

>    // I'm building a tree. I'm just marking the two nodes in the tree as 
> ambiguous.
>    st.index = x0.parse_param.pResult->makeList(x0.index, x1.index, 
> node_t::list);
        ^^^^^                                       ^^^^^     ^^^^^
        Same end result as before (due to the union) but more clearer in what I 
am doing
>    x0.parse_param.pResult->ambig(st.index); // Mark node as ambiguous
>    return st;
> }

>> Yes. That is what I am doing.
> 
> No, it is not.  You are not returning an index, but a merge_t.

Well, I disagree. Remember -  it is a union. Do a memory map. The end result is 
the same.
I just didn't write it clearly. The above is clearer and does the same thing.
It still might be the best way to do things, but you are the seasoned 
professional
with bison - I'm a newbie. I was attempting to make it "richer" by passing *in* 
an
additional argument that I felt should be common to any semantic value used,
not just in the merge case - to replace the lack of a "%merge-param".

I hope this clears things up.

PS: You're knowledge and understanding of flex/bison beats mine hands down.
    In that respect, I strive to understand your guidance. I also try to
    take any suggestions to heart. But there is usually more than one way
    to look at something. And if I do something in a non-standard confusing
    way, feel free to point it out. Otherwise I miss the opportunity to learn.
    I just speak up when I think I am misunderstood - not to argue.

Thanks for putting up with me :)



reply via email to

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