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

Create Tabular package #209

Merged
merged 3 commits into from
Nov 9, 2021
Merged

Create Tabular package #209

merged 3 commits into from
Nov 9, 2021

Conversation

knothed
Copy link
Contributor

@knothed knothed commented Oct 20, 2021

Create a Tabular package which contains the Tabular module and related stuff like LALR.

@knothed
Copy link
Contributor Author

knothed commented Oct 20, 2021

When this MR is merged, #200 would be obsolete.

@Ericson2314 @int-index I believe the one thing left here is renaming, and possibly moving, Lr1State. Any suggestions?

@Ericson2314
Copy link
Collaborator

ARRAY-NOTES has some grammar things too, maybe we should split it for now?

@knothed
Copy link
Contributor Author

knothed commented Oct 22, 2021

ARRAY-NOTES has some grammar things too, maybe we should split it for now?

Hmm I'd probably prefer documenting this somewhere in the code. Grammar.lhs already has parts of the ARRAY-NOTES as a comment - what do you think?

@Ericson2314
Copy link
Collaborator

@knothed That's better! Then I'd say leave where it was in this commit, and then convert to code comments in subsequent commit.

@@ -417,18 +417,21 @@ the spontaneous lookaheads in the right form to begin with (ToDo).
-----------------------------------------------------------------------------
Merge lookaheads

> -- TODO needs better name
> type Lr1State = ([Lr1Item], [(Name, Int)])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should do sth about the todo :D @Ericson2314

@knothed
Copy link
Contributor Author

knothed commented Oct 27, 2021

Could this be merged? I think we won't need #200 anymore after merging this.

@Ericson2314
Copy link
Collaborator

Oh sorry I lost track of this. I am fine with Lrt1State for now, and I think I am fine with the rest of this too. @int-index you have any thoughts?

@Ericson2314
Copy link
Collaborator

#200 I rebased to just be additional splits and the main part, so I am happy to keep it around for later for when we revisit that sort of stuff :)

@int-index
Copy link
Collaborator

I will review by Monday.

@int-index
Copy link
Collaborator

I have not reviewed by Monday. Shame on me. But I’m doing it right now.

@int-index
Copy link
Collaborator

LGTM. Thank you!

@int-index int-index merged commit ede899b into haskell:master Nov 9, 2021
@Ericson2314 Ericson2314 deleted the tabular branch November 10, 2021 20:30
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

3 participants