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

Remove POrd instance of PValue - add PPartialOrd #528

Merged
merged 5 commits into from
Jul 26, 2022
Merged

Conversation

TotallyNotChase
Copy link
Collaborator

Closes #526

@TotallyNotChase TotallyNotChase requested review from blamario and L-as July 22, 2022 12:00
Plutarch/Bool.hs Outdated
@@ -89,6 +90,18 @@ class PEq t => POrd t where
infix 4 #<=
infix 4 #<

-- | Partial ordering relation.
class PPartialOrd t where
pleq :: Term s t -> Term s t -> Term s PBool
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should these methods be Plutarch level functions instead?

Plutarch/Bool.hs Outdated
@@ -89,6 +90,18 @@ class PEq t => POrd t where
infix 4 #<=
infix 4 #<

-- | Partial ordering relation.
class PPartialOrd t where
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this become a superclass of POrd? It'd require adding all the instances everywhere else

@L-as
Copy link
Member

L-as commented Jul 22, 2022 via email

@blamario
Copy link
Collaborator

Wouldn't Rust's approach be suboptimal in UPLC, if we adopt pcompare :: Term s (a #-> b #-> PMaybe POrdering)? Or do you mean something else about Rust?

I imagine there won't be much user-written polymorphic code; when writing a contract people tend to know what type they're dealing with. It's only the library writers who will have to decide between PPartialOrd and POrd. Sorted lists and maps will have to insist on POrd. I think it's fine to move #<= to PPartialOrd and to leave POrd with nothing but additional laws, and perhaps with the pcompare method added.

@TotallyNotChase
Copy link
Collaborator Author

I also think moving things into PPartialOrd and having just POrd with a superclass relation and additional laws is good. I'm against adding pcompare though, due to performance concerns.

@TotallyNotChase
Copy link
Collaborator Author

Moved the POrd methods into PPartialOrd and made POrd an empty class with a superclass constraint on PPartialOrd.
Suggest the typeclass laws that should be put into the POrd haddock

Copy link
Member

@L-as L-as left a comment

Choose a reason for hiding this comment

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

LGTM, great!

@L-as L-as merged commit e23f2d9 into staging Jul 26, 2022
@TotallyNotChase
Copy link
Collaborator Author

@L-as wait we still needed to add laws?

@L-as
Copy link
Member

L-as commented Jul 26, 2022

@TotallyNotChase Well we can do that in another PR and it's not essential for 1.2 IMO.

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.

3 participants