Closed knothed closed 2 years ago
Why move GenUtils
to happy-grammar
? I didn’t expect to see it there.
True, GenUtils
preferably shouldn't exist as it doesn't belong to any package. I'll add its functions directly into the packages that need them.
@int-index GenUtils
is now back in happy
and will be fully removed bit by bit over the course of the following MRs.
That’s looking better, thanks!
I noticed that you also had to do some changes. For example, I see that hd
and tl
were moved into the Grammar
data type, and that #ifdef DEBUG
conditionals have been removed in a couple of places.
Are these the only changes you had to do or is there something I missed? It would be easier for me to tell if the diff looked like this:
error_handler :: Maybe String,
error_sig :: ErrorHandlerType,
+ hd :: Maybe String,
+ tl :: Maybe String
}
But instead I see the Grammar
data type removed and re-added in its entirety in another file but with slight modifications.
For this reason I would like to see the PRs to come in one of two flavors:
hd
and tl
to Grammar
)Apologies about adding more busywork to this already huge undertaking, but it’s truly not easy to review otherwise.
Okay, so I'll try splitting this and the following MRs into two MR's each: one where code is moved, but not changed, and one where code is changed, but not moved. I think it should be possible to do so, but I can't guarantee a 100% success.
Of course, moving code around will impact module
and import
statements.
Done. Now the MR only contains pure code movements, namely the following:
Grammar
, Production
, Assoc
, Priority
: Mangler -> GrammarmapDollarDollar
: GenUtils -> GrammarErrorHandlerType
: AbsSyn -> GrammarmkClosure
: GenUtils -> Firststr
, interleave
, interleave'
: GenUtils -> Info (copy)die
, dieHappy
, getProgramName
, optPrint
: Main -> GenUtilsdie
: Main -> TabularLRAction
, Goto
, ActionTable
, GotoTable
: Mangler -> Tablesfind_redundancies
, is_shift
: Main -> FindRedundanciesany_reduction
, first_reduction
: Main -> TabularAlso, the following wrapper functions around otherwise purely moved code have been created:
reportUnusedRules
, reportConflicts
, writeInfoFile
, runTabular
: Main -> TabularIs there more feedback or are you happy with the MR? @int-index @Ericson2314
It’s in my queue.
I tried to run git diff master --color-moved=blocks -w
locally to confirm that this MR just moves things around, but git isn’t smart enough to ignore changes to indentation and migration from .lhs
to .hs
Didn’t want to bother you with more nitpicking and decided to rearrange a few things myself (preserving attribution in parts that you’ve done, of course), but have been busy with other things.
I’ll try to get this over with by the end of the week.
@int-index ping, what do you want to do?
Oops! I’ll do it today.
Thanks!
OK, here’s my take on this: #203
The key difference from this patch is that my genTables
is pure, unlike the runTabular
here. I think it’s up to Main.lhs
to do the actual reporting and to dump the info file, that’s part of the CLI UI basically.
In the happy-tabular
package, we only want the logic, not the UI.
OK I rebased this, and ended keeping "both" tabulars -- temporarily.
I didn't realize it at first, but we got back to the issue of @knothed wanting the "main fragments" vs not putting such things in the libraries. It's tough because I see the real use in happy-rads on one hand, but on the other hanging random clumps of IO in libraries violates many fundamental and important rules about how libraries ought to distribute labor. I get really nervous violating those rules.
If we are to do the more tortoise-like "little steps, good review" method, vs more hare-like "big PR clean up after", then I think it is best to not do the main fragments, and keep the IO and CLI parsing in Main. I do want to do something for happy-rads, but it just is premature to try to plan that at this stage of the "little steps, good review" process.
So my rebase keeping the main fragment is just temporary so that we can get it on some other branch and not loose the work. I had pinged @int-index thinking we are close, but now I am afraid it doesn't feel as close. I am pushing what I got because I am at the limit I have time to can spend rebasing this stuff for the next week or so, but hopefully between the 3 of us we can keep this moving.
OK added the checklist.
I've just opened #205. I don't know if that's what you had in mind @Ericson2314, but I think pulling out the grammar is a step in the right direction.
Okay, the next step would be to put tabular-stuff in the happy-tabular package in a new MR, right?
I rebased this on top of #209, so the top commit is all the additional changes we decided to hold off on for now, hope that is of some use @knothed.
OK if there is anything we want to "mine" from here, it should be easy enough to do so.
I’m fine with tabular
and grammar
as they are in master
. I think we can start moving the backend/frontend into their own packages.
Before moving the GLR backend though, I think it would be better to make it a pure function (like the normal backend).
Yeah this can sit on the back burner. On on hand we could do the module splits but it doesn't really matter. On the other hand we probably don't want the main thing any time soon but having this here just in the PR might help @knothed keep happy-rad building in the short-term.
191
Create
happy-grammar
andhappy-tabular
.TODO:
happy-grammar
Happy.Tablular
in a different branch for now, and callHappy.Tablular.Tablular
Happy.Tablular
instead --- we'll get back to such "main fragments" later.Lr1State
a better name (note where I moved it, unfortunately breaking the symmetry withLr0State
).