Skip to content

Conversation

@justinwoo
Copy link
Contributor

I realized that I need this function but we don't seem to have it yet

@paf31
Copy link
Contributor

paf31 commented Jul 30, 2017

This looks good, but I wonder if we should add this now, or try to figure out how to add this to Prelude as an Eq instance.

@justinwoo
Copy link
Contributor Author

I could move this over to a Prelude PR once things are in place there

, RowCons name ty tailRow row
, EqualFields tail row
) => EqualFields (Cons name ty tail) row where
equalFields _ a b = get' a == get' b && rest

Choose a reason for hiding this comment

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

hey @justinwoo! I think it might be more efficient to use an if expression to make this short-circuit:

  equalFields _ a b =
    if get' a == get' b
      then equalRest a b -- evaluate this fully only inside the true branch
      else false
    where
      get' = get (SProxy :: SProxy name)
      equalRest = equalFields (RLProxy :: RLProxy tail)

(I think the strictness means that the rest of the record would be compared before being passed to &&)

Copy link
Member

Choose a reason for hiding this comment

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

get' a == get' b && equalRest a b would also short circuit :)

Choose a reason for hiding this comment

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

Oh, okay! Sometimes I forget we have compiler magic – thanks! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated! thanks

@paf31
Copy link
Contributor

paf31 commented Sep 9, 2017

@justinwoo Could you please rebase this?

@justinwoo
Copy link
Contributor Author

rebased!

@paf31 paf31 merged commit d39eb6e into purescript:master Sep 10, 2017
@paf31
Copy link
Contributor

paf31 commented Sep 10, 2017

Thanks!

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.

4 participants