-
Notifications
You must be signed in to change notification settings - Fork 501
valueContains: enforce no negative amounts in either Value #7376
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
Conversation
80608be to
cf4bf3d
Compare
| {- ^ Total size, i.e., sum total of inner map sizes. This avoids recomputing | ||
| the total size during the costing of operations like `unionValue`. | ||
| -} | ||
| {-# UNPACK #-} !Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, neat idea!
| Maybe Int | ||
| -> ( -- Left (old size of inner map) if the total size grows by 1, | ||
| -- otherwise, Right (old amount) | ||
| Either Int Integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the concept of "boolean blindness", I prefer to define my own, equivalent, datatypes in these situations.
So, in this case, instead of writing a comment which documents what Either Int Integer represents, you can have the code document itself if you define a type data Growth = OldInner Int | OldAmount Integer (very bad names, but you get the idea).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very local - used only in a local function inside a function, so I'd very much rather not add a dedicated top-level data type just for this.
| Just old -> (updateSizes old (old + 1) sizes, size + 1) | ||
| Nothing -> (sizes, size) | ||
| in pure $ Value outer' sizes' size' | ||
| in (maybe (Left (Map.size inner)) Right moldAmt, Just inner') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above, this combination of Maybe, Either and tuples makes it hard to understand what is going on without running the whole function in your head.
Unisay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Per discussion and consensus in cardano-foundation/CIPs#1088