skulpt / skulpt_parser

Experiments with python 3.9 pegen parser for skulpt
Other
6 stars 4 forks source link

Symtable #91

Closed albertjan closed 3 years ago

albertjan commented 3 years ago

@s-cork all the visit stuff is done now analyse remains

albertjan commented 3 years ago

left a few comments for you

albertjan commented 3 years ago

addressed most of the comments 😄 few outstanding q's.

Lets think about how we can put this through it's paces

github-actions[bot] commented 3 years ago

test result: ok. 559 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (23476ms)

github-actions[bot] commented 3 years ago

test result: ok. 559 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (31930ms)

albertjan commented 3 years ago

it appears we broke the ast tests at some point haha we didn't actually exit with an error code 😄

s-cork commented 3 years ago

oh no so blissfully unaware

s-cork commented 3 years ago

the dump function errors are escape characters for strings so we can just make that a separate pr fix

albertjan commented 3 years ago

Alright it generates a SymbolTable for all of the run-tests, now the small matter of proving that is the correct symbol table 😬

albertjan commented 3 years ago

Another good point if you're talking to Anvil about typescript. I wrote all of this without running it once. And I only had to change a few little things.

albertjan commented 3 years ago

Why not do a call inside SEQ instead? bind is expensive

    SEQ<T>(visitor: (node: T) => void, nodes: (T | null)[]) {
        assert(Array.isArray(nodes), "SEQ: nodes isn't array? got " + nodes.toString());
        for (const node of nodes) {
            if (node) {
                visitor.call(this, node);
            }
        }
    }

thanks!

albertjan commented 3 years ago

ls -1 run-tests/*.py | xargs -n1 vr symtable if you want to see it in action

s-cork commented 3 years ago

script hasn't been pushed yet...

albertjan commented 3 years ago

@s-cork tests pass 😄

s-cork commented 3 years ago

lgtm - merge this and then create new prs for whatever comes next?

albertjan commented 3 years ago

Yeah sounds good to me.