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

Add the ByteSize measure #490

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Add the ByteSize measure #490

wants to merge 4 commits into from

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Jul 3, 2024

The new ByteSize measure should be used for all byte size summations relevant to the Cardano chain.

  • It has checked arithmetic, with a pure representation of overflow.
  • It still has a Monoid instance that will be useful for finger trees.

The other types defined in the module should be used for recording/transmitting the result of summations that did not overflow.

@nfrisby nfrisby requested a review from lehins July 3, 2024 20:17
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 3, 2024

TODO test suite

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

I took a quick glance at it. Looks interesting.
I'll do a proper review after the Chang HF, if that is ok?

-- case above.
class ByteSizePartialFrom a where
-- | See 'ByteSizePartialFrom'.
partialFrom :: a -> StrictMaybe ByteSize
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't really call it partial, since partial functions in Haskell usually are the ones that produce exceptions, eg. error is a partial function. I get it that math has a different view on the subject, but in order to stick to Haskell nomenclature I'd call it something like toByteSizeMaybe

DefaultSignatures can reduce some boiler plate

Suggested change
partialFrom :: a -> StrictMaybe ByteSize
partialFrom :: a -> StrictMaybe ByteSize
default partialFrom :: ByteSizeFrom a => a -> StrictMaybe ByteSize
partialFrom = partialFromDefault

-- | Law: @'partialFrom' = 'SJust' . 'from'@
class ByteSizePartialFrom a => ByteSizeFrom a where
-- | See 'ByteSizeFrom'.
from :: a -> ByteSize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not only to and from are very confusing and ambiguous names, but they are also highly likely to be used in user's codebase. Without qualification these will lead to clashes quite often (even lens/microlens has to)

I'd suggest better names that depict what is the actual conversion here. So in order to avoid ambiguity of what is being converted to what, I'd suggest something like this:

-- | A type that can be safely converted to `ByteSize`
class ToByteSize a where
    toByteSize :: a -> ByteSize

-- | A type that can be safely constructed from `ByteSize`
class FromByteSize a where
    fromByteSize :: a -> ByteSize

Copy link
Contributor Author

@nfrisby nfrisby Jul 4, 2024

Choose a reason for hiding this comment

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

Without qualification ...

So far, I'm intending for this module to be imported qualified.

Example: ByteSize.from @Word32 1000

Copy link
Contributor

@coot coot Jul 5, 2024

Choose a reason for hiding this comment

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

Do we need to export SJust . from at all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So far, I'm intending for this module to be imported qualified.

I appreciate your intention, but that should not be forced on the user. Moreover, qualification does not solve the ambiguity of naming from and to.

So, to put it bluntly, I will not let functions with names like from and to to be merged.

Comment on lines +53 to +56
#if __GLASGOW_HASKELL__ < 900
-- Use the GHC version here because this is compiler dependent, and only indirectly lib dependent.
import GHC.Natural (Natural)
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why CPP here?

Suggested change
#if __GLASGOW_HASKELL__ < 900
-- Use the GHC version here because this is compiler dependent, and only indirectly lib dependent.
import GHC.Natural (Natural)
#endif
import Numeric.Natural (Natural)

Copy link
Contributor Author

@nfrisby nfrisby Jul 4, 2024

Choose a reason for hiding this comment

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

I don't know; I merely copied it from an existing module in the package.

Edit: seems like the CI does not like it 🤔

-- | NB the result will never be @'SJust' 'maxBound'@.
instance ByteSizeTo Word64 where to = toBiggerIntegral

instance ByteSizePartialFrom Natural where partialFrom = partialFromDefault
Copy link
Contributor

Choose a reason for hiding this comment

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

This could overflow, isn't it? Shouldn't we do sth like this here:

 \a -> if a >= fromIntegral sentinel then SNothing else ...

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