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

Fix getRecordSelectors at the source #55

Merged
merged 5 commits into from
Apr 11, 2017
Merged

Fix getRecordSelectors at the source #55

merged 5 commits into from
Apr 11, 2017

Conversation

RyanGlScott
Copy link
Collaborator

This migrates the changes I originally put forth in goldfirere/singletons#181 to the site of getRecordSelectors' definition. getRecordSelectors now requires a list of DCons instead of just one, which should make it harder to trigger the bug in goldfirere/singletons#180.

Copy link
Owner

@goldfirere goldfirere left a comment

Choose a reason for hiding this comment

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

As always, I'm very grateful for these contributions. If you've got the time, I'd appreciate the changes below. If not, feel free to merge. Thanks!

varName <- qNewName "field"
let tvbs = fvDType arg_ty
maybe_forall
| S.null tvbs = id
Copy link
Owner

Choose a reason for hiding this comment

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

Why the special case? An empty forall is OK.

varName <- qNewName "field"
let tvbs = fvDType arg_ty
maybe_forall
| S.null tvbs = id
Copy link
Owner

Choose a reason for hiding this comment

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

OK. Now I realize that you're just copying my code. But I still think an empty forall is OK.

-- their clauses.
| DFunD n clauses <- x
= let (other_clauses, neq_n)
= partitionWith (\case DFunD n2 cls | n == n2 -> Left cls
Copy link
Owner

Choose a reason for hiding this comment

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

This is quadratic. :( Would it work to use a finite map (that is, Data.Map) mapping names to lists of clauses? That seems much more efficient.

@@ -273,6 +273,15 @@ mapMaybeM f (x:xs) = do
Nothing -> ys
Just z -> z : ys

-- like GHC's
partitionWith :: (a -> Either b c) -> [a] -> ([b], [c])
Copy link
Owner

Choose a reason for hiding this comment

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

It's a shame this isn't in some library. Very useful function.

@RyanGlScott
Copy link
Collaborator Author

@goldfirere, thanks for the review. I've updated the internals of getRecordSelectors so that it isn't quadratic anymore, and rejigged the documentation appropriately. Does its new incarnation look OK?

Copy link
Owner

@goldfirere goldfirere left a comment

Choose a reason for hiding this comment

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

Looks good, except for a minor suggested refactoring. After the refactor, feel free to merge directly. Thanks!

then gather_decs (M.insertWith (\new old -> old ++ new)
n clauses name_clause_map)
type_sig_names xs
else let (map', decs') = gather_decs (M.insert n clauses name_clause_map)
Copy link
Owner

Choose a reason for hiding this comment

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

The previous call to gather_decs will work here, right? If so, perhaps bind that result somewhere, so it doesn't look like the map-insertion is changing between the then and else.

@RyanGlScott RyanGlScott merged commit 88a25d5 into goldfirere:master Apr 11, 2017
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.

2 participants