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

Initial commit for a cpp-peglib based parser and untyped AST gen #116

Merged
merged 8 commits into from
May 19, 2020

Conversation

sylvanc
Copy link
Member

@sylvanc sylvanc commented May 19, 2020

This uses cpp-peglib to generate a packrat PEG for a grammar defined
in a text file, and then does some very basic AST manipulation. It
sets up scopes and symbol tables, and does a bit of fixity analysis.
The intention is to use this for a very slim parser front-end that
goes to MLIR before any serious heavy lifting is done. No tests are
included in the initial commit, as the grammar.peg is preliminary.Z

This uses cpp-peglib to generate a packrat PEG for a grammar defined
in a text file, and then does some very basic AST manipulation. It
sets up scopes and symbol tables, and does a bit of fixity analysis.
The intention is to use this for a very slim parser front-end that
goes to MLIR before any serious heavy lifting is done. No tests are
included in the initial commit, as the grammar.peg is preliminary.Z
@mjp41
Copy link
Member

mjp41 commented May 19, 2020

Can you use

https://github.com/yhirose/cpp-peglib.git

as the submodule reference, rather than an ssh based one.

@rengolin
Copy link
Contributor

Quick overall comment, there are a number of assumptions (make_shared onto nullptr, fields were set before use) that could segfault without errors.

I think adding user errors for those is wrong, because it's not a user error, so assert would be the right level, I think. CMake has release builds with assert, which are good for testing that kind of thing.

@rengolin
Copy link
Contributor

The lack of tests should be fine, given the state of the grammar and the prototype. I think this can be the next task after merge, to go through the list of tests we already have and make sure they have the syntax we want and change the parser/tests to match it.

This will be a long process, with a lot of discussion, and can happen in parallel with other work (ex. the MLIR bridge, runtime libraries).

@rengolin
Copy link
Contributor

Initial list of files from the testsuite (marked as compile-pass) that this front-end parses into AST:

./resolution/compile-pass/circular.verona
./resolution/compile-pass/interleaved.verona
./reachability/compile-pass/visit-variable-renaming-type.verona
./reachability/compile-pass/visit-receiver.verona
./features/compile-pass/library/sibling.verona
./features/compile-pass/cell.verona
./typechecking/compile-pass/field-write.verona

None of the run-pass parses.

It seems most (if not all) errors are on class declaration.

This is just a comment, for future reference, doesn't preclude merging the code as is.

Copy link
Contributor

@rengolin rengolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more inline comments, but I'm done. I think this is a straight forward implementation of a parser that despite being in a raw state is actually a good start for what we want to do with it. If we had a fully tested parser now, we'd have to keep changing tests to accommodate the evolving language.

Right now, it's simple enough that will be easy to change either parser or examples to suite our design process, and that's the best of both worlds. So, to me, it looks good to merge. Any comments above could be implemented post-merge without needing to withhold this patch.

src/ast/main.cc Outdated Show resolved Hide resolved
src/ast/path.h Show resolved Hide resolved
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start for this.

src/ast/path.h Show resolved Hide resolved
@mjp41
Copy link
Member

mjp41 commented May 19, 2020

Actually can we remove it from building in CI. I don't really want failed CI ever on master.

@mjp41 mjp41 merged commit 180d646 into master May 19, 2020
@mjp41 mjp41 deleted the sylvanc-ast branch May 19, 2020 19:58
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.

None yet

4 participants