-
Notifications
You must be signed in to change notification settings - Fork 17
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
Replace ocamlyacc with cpspg #165
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This PR opens new perspectives for a better parser. However, I cannot merge it yet, since it breaks CI.
(ocamllex (modules Lexer)) | ||
;(ocamlyacc (modules YaccParser)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this line instead of commenting it out? As discussed on the last meeting, we are going to use nice features of cpspg (which are not implemented yet), so I think in the near future, we will be incompatible with ocamlyacc.
src/DblParser/dune
Outdated
|
||
(rule | ||
(deps YaccParser.mly) | ||
(target YaccParser.ml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does cpspg generate .mli
files? If yes, maybe it should be added to this rule (I'm sure, dune can do that. You can ask @Foxinio, our local dune guru). If not, maybe we could consider writing own YaccParser.mli
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not generate .mli
files, but it should be straightforward to implement.
I got CI to work using nix here. I packaged both
|
Co-authored-by: ilikeheaps <[email protected]>
|
||
export DBL_LIB="lib/" | ||
|
||
TIMEOUT=1.0 | ||
|
||
binary="_build/default/src/$1.exe" | ||
binary=$1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a minor problem with this change. When I run tests locally, I would like to use a version of dbl compiled from source on the active branch, not the one that is installed on my system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of an oversight on my part - in my dev environment, dbl
in PATH
is the one from _build/...
. To be fair, you still can run ./test.sh ./_build/default/src/dbl.exe ./test/test_suite
to test against your freshly built dbl
, but I understand that the default workflow should probably remain unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restored most of the original behaviour of test.sh
in my branch - when passed a relative path as PROGRAM
it does what it used to do.
This PR replaces ocamlyacc with fram-lang/cpspg@67dfc0f to check its compatibility and experiment with the new parser generator tool.
Building now requires
cpspg
to be in the path - it can bedune build
anddune install
-ed from adampsz/cpspg repo.