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

Incorrect railroad diagram #66

Open
aDotInTheVoid opened this issue Sep 24, 2023 · 3 comments · May be fixed by #67
Open

Incorrect railroad diagram #66

aDotInTheVoid opened this issue Sep 24, 2023 · 3 comments · May be fixed by #67

Comments

@aDotInTheVoid
Copy link

$ cat ./eg.ebnf
binding-names = {name, ","}, name, ",", name, [","];
$ ./build/bin/kgt -l iso-ebnf -e rrutf8 < ./eg.ebnf
binding-names:
                      ╭────>────╮
                      │         │
    │├──╭── name ──╮──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

However this diagram isn't accurate, as it permits just "name", which isn't allowed by the grammar. The correct output would look like

binding-names:
                                     ╭────>────╮
                                     │         │
    │├──╭── name ──╮── "," ── name ──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

This may be related to #61 as they both have very similar inputs.

@aDotInTheVoid
Copy link
Author

Looking at this a bit more:

./build/bin/kgt -l iso-ebnf -e dot < ./eg.ebnf | dot -Tpng > dot.png

image

Which looks about right to my untrained eye, although the bottom left "name" not being grey and square may be something. Not sure what this means.

./build/bin/kgt -l iso-ebnf -e rrdot  < ./eg.ebnf | dot -Tpng > rr.png

image

This is defiantly wrong, as it allows doesn't require 2 "name"s

When running with rrd_pretty disabled, it generate's

./build/bin/kgt -l iso-ebnf -e rrdot -n < ./eg.ebnf | dot -Tpng > dot.png

image

which seems right.

This is further confirmed by producing a diagram without rrd_pretty:

$ ./build/bin/kgt -l iso-ebnf -e rrutf8 -n  < ./eg.ebnf
binding-names:
                                                             ╭───────>───────╮
                                                             │               │
    │├─────╭───────────────────────╮── name ── "," ── name ──╯───── "," ─────╰─────┤│
           │                       │
           ╰───── "," ── name ─────╯

which is deffinatly right.

I think the next step is for me to to modify rrd_pretty so I can dump a diagram after each pass, so see which one in the culperate.

@aDotInTheVoid aDotInTheVoid linked a pull request Sep 25, 2023 that will close this issue
@aDotInTheVoid
Copy link
Author

I've modified rrd_pretty to dump the output between each pass, to try to find the cuplerit. Annoyingly I couldn't use rrdot for this, because rrdot_output intersperces ast_to_rrd rrd_pretty and actually doing the IO. Instead I've used rrutf8, which while not showing the full AST, does help here.

All output is from 0e405d3

$ ./build/bin/kgt -l iso-ebnf -e rrutf8 < ./bad1.ebnf 
Just ran collapse
                                                    ╭────>────╮
                                                    │         │
    │├──╭─────────────────╮── name ── "," ── name ──╯── "," ──╰──┤│
        │                 │
        ╰── "," ── name ──╯

Just ran skippable
                                                    ╭────>────╮
                                                    │         │
    │├──╭─────────────────╮── name ── "," ── name ──╯── "," ──╰──┤│
        │                 │
        ╰── "," ── name ──╯

Just ran redundant
                                                    ╭────>────╮
                                                    │         │
    │├──╭─────────────────╮── name ── "," ── name ──╯── "," ──╰──┤│
        │                 │
        ╰── "," ── name ──╯

Just ran collapse
                                                    ╭────>────╮
                                                    │         │
    │├──╭─────────────────╮── name ── "," ── name ──╯── "," ──╰──┤│
        │                 │
        ╰── "," ── name ──╯

Just ran roll
                                     ╭────>────╮
                                     │         │
    │├──╭── name ──╮── "," ── name ──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran collapse
                                     ╭────>────╮
                                     │         │
    │├──╭── name ──╮── "," ── name ──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran nested
                                     ╭────>────╮
                                     │         │
    │├──╭── name ──╮── "," ── name ──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran ci
                                     ╭────>────╮
                                     │         │
    │├──╭── name ──╮── "," ── name ──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran collapse
                                     ╭────>────╮
                                     │         │
    │├──╭── name ──╮── "," ── name ──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran affixes
                      ╭────>────╮
                      │         │
    │├──╭── name ──╮──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran collapse
                      ╭────>────╮
                      │         │
    │├──╭── name ──╮──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran bottom
                      ╭────>────╮
                      │         │
    │├──╭── name ──╮──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

Just ran collapse
                      ╭────>────╮
                      │         │
    │├──╭── name ──╮──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

binding-names2:
                      ╭────>────╮
                      │         │
    │├──╭── name ──╮──╯── "," ──╰──┤│
        │          │
        ╰── "," ───╯

This suggests that the problem is in rrd_pretty_affixes

@edk0
Copy link
Collaborator

edk0 commented Sep 25, 2023

Yeah, it seems designed to work on a completely rrd_pretty_roll'd tree.

It seems like it would translate

||-->-- A -- B --v-- C -- D -- A -- B --||
    |            |
    ^-- D -- C --<

to

||-->-- A -- B --v--||
    |            |
    ^-- D -- C --<

which would be correct if it incremented the loop counter. Except... that can't ever happen, because rrd_pretty_roll+ would have converted the first diagram to

||-->-- A -- B -- C -- D --v-- A -- B --||
    |                      |
    ^----------------------<

so I'm a bit confused about what it's trying to do. Can you think of an example where it changes anything? (Assuming #67, of course).

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 a pull request may close this issue.

2 participants