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.

Sign up for GitHub

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

Jump to bottom

Plutarch Numeric hierarchy #160

Draft
wants to merge 13 commits into
base: staging
Choose a base branch
from
Draft

Plutarch Numeric hierarchy #160

wants to merge 13 commits into from

Conversation

sergesku
Copy link
Contributor

No description provided.

L-as
L-as requested changes Jan 21, 2022
View reviewed changes
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.

In general looks good, very few missed hoists, etc., although I'm not really sure why you're removing the Num instances?

Plutarch/Api/V1.hs Outdated
@@ -290,8 +290,6 @@ deriving via

newtype PPOSIXTime (s :: S)
= PPOSIXTime (Term s PInteger)
deriving (PIntegral) via (PInteger)
Copy link
Member

Choose a reason for hiding this comment

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

?

Plutarch/Builtin.hs Outdated
@@ -228,15 +228,15 @@ instance PIsData PBool where
(phoistAcyclic $ plam toBool) # pforgetData x
where
toBool :: Term s PData -> Term s PBool
toBool d = pfstBuiltin # (pasConstr # d) #== 1
toBool d = pfstBuiltin # (pasConstr # d) #== pconstant 1
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Plutarch/DataRepr.hs Outdated
@@ -84,7 +95,7 @@ pindexDataList n =
punsafeIndex @PBuiltinList @PData # ind
where
ind :: Term s PInteger
ind = fromInteger $ natVal n
ind = pconstant $ natVal n
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Plutarch/Integer.hs Outdated
instance PEq PInteger where
x #== y = punsafeBuiltin PLC.EqualsInteger # x # y

instance POrd PInteger where
x #<= y = punsafeBuiltin PLC.LessThanEqualsInteger # x # y
x #< y = punsafeBuiltin PLC.LessThanInteger # x # y

instance Num (Term s PInteger) where
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this instance?

Plutarch/Integer.hs Outdated
ppowInteger :: PMultiplicativeGroup a => Term s a -> Term s PInteger -> Term s a
ppowInteger a int =
phoistAcyclic
( plam $ \x i ->
Copy link
Member

Choose a reason for hiding this comment

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

do-syntax please

Copy link
Member

Choose a reason for hiding this comment

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

We can't actually use do-syntax, because it breaks GHC 8.10.7...

Plutarch/Integer.hs Outdated
forall s a.
(PMultiplicativeMonoid a) =>
Term s (a :--> PInteger :--> a)
pexpBySquaring = pfix #$ plam f
Copy link
Member

Choose a reason for hiding this comment

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

This isn't hoisted.

Plutarch/List.hs Outdated
go = (pfix #$ plam $ \self ls n -> pelimList (\_ xs -> self # xs # n + 1) n ls)
in go # xs # 0
go = (pfix #$ plam $ \self ls n -> pelimList (\_ xs -> self # xs # n #+ pone) n ls)
in go # xs # pzero
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Plutarch/Natural.hs Outdated

-- We made sure that PInteger >= zero as it goes from PNatural.
pnatFromIntUnsafe :: Term s PInteger -> Term s PNatural
pnatFromIntUnsafe = pcon . PNatural
Copy link
Member

Choose a reason for hiding this comment

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

In general it should be punsafe*.

Plutarch/Natural.hs Outdated

instance PIsData PNatural where
pfromData x =
phoistAcyclic
Copy link
Member

Choose a reason for hiding this comment

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

You can just use pfromData for integers here without all this complex logic. The argument passed to this function is PAsData PNatural, so we already know it's correct. No need to check anything.

Plutarch/Natural.hs Outdated
PNothing -> perror
)
# x
pdata x =
Copy link
Member

Choose a reason for hiding this comment

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

No need for the unnecessary lambda here either.

@sergesku
Copy link
Contributor Author

sergesku commented Jan 21, 2022 •
edited
Loading

I'm not sure about Num and PIntegral instances.
I get the task from @kozross to replace Num and Fractional instances with the new Numeric hierarchy.

And I must admit, using pconstant on numeric literals all the time is a pain.

@L-as
Copy link
Member

L-as commented Jan 21, 2022

I think it's good to have the new classes, but I don't see why Num should be removed. It's quite convenient.

@sergesku
Copy link
Contributor Author

sergesku commented Jan 21, 2022 •
edited
Loading

OK, I'll return them back

@TotallyNotChase TotallyNotChase mentioned this pull request Feb 17, 2022
Closed
@L-as L-as marked this pull request as draft February 21, 2022 20:02
@TotallyNotChase
Copy link
Collaborator

I'm trying to get this ready for Plutarch 1.2, as part of plutarch-extra.

@L-as L-as marked this pull request as ready for review April 13, 2022 17:04
@L-as
Copy link
Member

L-as commented Apr 25, 2022

@TotallyNotChase is this ready?

@TotallyNotChase
Copy link
Collaborator

@L-as Not yet, I'm going to do some finishing touches post #435 and then we should get a review from Koz and fill in any missing pieces.

@TotallyNotChase TotallyNotChase marked this pull request as draft April 26, 2022 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@L-as L-as L-as requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sergesku @L-as @TotallyNotChase