-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add F# support via fslex and fsyacc #404
base: master
Are you sure you want to change the base?
Conversation
@andreasabel what is your opinion on adding F# support via From my point of view, it would bring back dotnet to the table for bnfc after removing C# and compared to |
Hello @gdziadkiewicz, thank you for your initiative! I was planning to evaluate your PR yesterday, but Let's start discussion anyway, here are some points:
P.S.: Good you got rid of |
p.s. Having a reliable, maintained interpolation-providing package that's not deps heavy would be really nice. Did you like that |
Differences between fslex/fsyacc and ocamllex/ocamlyacc:
|
@gdziadkiewicz worte:
Great! This pledge is nothing formal, it is the serious promise that you will maintain the F# backend for at least 3 years.
I don't really know if something can be salvaged, but I thought I point this out to you in case you want to do the archeology to see how things were done then.
Great!
Excellent. I am hoping to make some progress on this early next year.
Yes, the testsuite is the most important thing. This might also solve point 6, if you provide a script or instructions that install dotnet and the required tools. To integrate the testsuite into CI is also a project I have been postponing for too long already, but maybe it can wait until BNFC3. I have not made any concrete plans how to migrate the testsuite, but I am resolved to migrate it one way or the other.
It is not the fault of dotnet, but the fault of my homebrew setup, so no need to apologize. But a pointer to get started would still be nice.
No, I passed by there, but did not find a "shopwindow" that would get me interested (see nikita-volkov/neat-interpolation#32). |
|
Work on getting all the existing tests passing still in progress. 23/30 of the parametrized tests are passing. During the process, I'm making notes on the fragile, easy-to-break parts of the code that should be replaced during the rewrite. |
|
I'm done with the tests and started working on the new version which parametrizes the OCaml backend. |
Thanks for the PR! I built and tested it. The first is
This is probably due to missing sanitization, if the user uses
The other test is
|
@gdziadkiewicz Currently the generated
|
I'm sorry for the long silence. I added the missing goals, but have some questions about them as I rarely use |
testParserRule = Makefile.mkRule tgt deps [ "dotnet build" ] | ||
where | ||
tgt :: String | ||
tgt = buildTarget opts |
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 used a name that is not a file name. Is this a problem?
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.
Well, if the target is a file, then make
can use its time-stamp to decide on rebuilding.
But I guess you have your reasons to not put a file there (maybe it is too complicated or fragile).
I am OK with that.
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.
For a moment I believed that there won't be a common file name for Linux and Windows but this turned out to be false. I will fix it.
phonyRule = vcat | ||
[ "# List of goals not corresponding to file names." | ||
, "" | ||
, Makefile.mkRule ".PHONY" [ "all", "clean", "distclean" ] [] |
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.
My testParserRule also does not correspond to a file name. Should I add it here?
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.
Yes, please.
mkFile withLang "Test" "" opts, | ||
mkFile withLang "" "fsproj" opts, | ||
utilFile opts, | ||
makeFile ]] |
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 used the code from Haskell as a reference for this part, but I don't understand why it removes the makefile.
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.
This is indeed debatable. I think the rationale was that distclean
should remove all generated files, and the Makefile
was one of them. However, this could be dangerous if the user edited the Makefile
later. Atm, the user also has to edit the distclean
goal then.
Personally, I do not use the distclean
, but there is a test for it in bnfc-system-tests
. Thus, I would just keep it like this.
A smarter cleaning logic would be another project: store hashes along generated files and only automatically delete stuff that hasn't changed since generated last (checkable using the hash)...
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 for the update!
testParserRule = Makefile.mkRule tgt deps [ "dotnet build" ] | ||
where | ||
tgt :: String | ||
tgt = buildTarget opts |
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.
Well, if the target is a file, then make
can use its time-stamp to decide on rebuilding.
But I guess you have your reasons to not put a file there (maybe it is too complicated or fragile).
I am OK with that.
phonyRule = vcat | ||
[ "# List of goals not corresponding to file names." | ||
, "" | ||
, Makefile.mkRule ".PHONY" [ "all", "clean", "distclean" ] [] |
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.
Yes, please.
mkFile withLang "Test" "" opts, | ||
mkFile withLang "" "fsproj" opts, | ||
utilFile opts, | ||
makeFile ]] |
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.
This is indeed debatable. I think the rationale was that distclean
should remove all generated files, and the Makefile
was one of them. However, this could be dangerous if the user edited the Makefile
later. Atm, the user also has to edit the distclean
goal then.
Personally, I do not use the distclean
, but there is a test for it in bnfc-system-tests
. Thus, I would just keep it like this.
A smarter cleaning logic would be another project: store hashes along generated files and only automatically delete stuff that hasn't changed since generated last (checkable using the hash)...
Five years ago I used bnfc (it's amazing, big thanks to all ppl involved in creating it) for the first time to generate frontend for my first compiler. Time issues didn't allow me to add F# support first and in the end, I manually adjusted the OCaml output to work with F#. For the five recent years, I made numerous attempts to finalize the F# support PR, but there was always something that made me fail and start from the beginning. Well, not anymore. Here I am sharing something that should have been shared a lot quicker.
As
fslex
is a port ofocamllex
andfsyacc
ofocamlyacc
the code is based on OCaml code.TODO list:
confirm if it is ok to add new dependencyinterpolate
, if not remove itifinterpolate
gets a green light then use it to make boilerplate code more readable using itifinterpolate
gets a green light then consider removingstring-qq
as it can be replaced byinterpolate
EDIT:
Due to problems with building on GHC 9.2.1 I decided to remove
interpolate
. It (orstring-interpolate
) can always be added later, in separate PR, and used to refactor.During the work, I plan to make notes of the differences between fslex/fsyacc and ocamllex/ocamlyacc for future reference.