-
Notifications
You must be signed in to change notification settings - Fork 157
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
Added NonZero
#4837
Added NonZero
#4837
Conversation
48b1e81
to
930bddd
Compare
f0f71ea
to
98ad7eb
Compare
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.
I understand you have good intentions, but this PR instead of introducing a simplification, as was intended by #4696, makes all those places where %? was suppose to be used even more complicated.
Please, go through my suggestions and I'll review the PR again.
Don't get me wrong, I really do like the NonZero
interface that you implemented in this PR, except this interface needs to be used only in places where we can guarantee a non zero value! securityParameter
is a perfect example of such use case, because we can guarantee that it is non-zero. NOpt
will be another, but only once we change its serialization, not before.
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes/NonZero.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes/NonZero.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes/NonZero.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes/NonZero.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/Conway/Instances/Basic.hs
Outdated
Show resolved
Hide resolved
libs/cardano-protocol-tpraos/src/Cardano/Protocol/TPraos/BHeader.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState/PulsingReward.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes/NonZero.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState/PulsingReward.hs
Outdated
Show resolved
Hide resolved
822132f
to
043541c
Compare
3c5c104
to
ce1ba92
Compare
libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes/NonZero.hs
Outdated
Show resolved
Hide resolved
bd85ba2
to
b6921de
Compare
07943f2
to
db675aa
Compare
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.
Beautiful!
Thank you!
e314307
to
d5da0c7
Compare
Co-authored-by: Alexey Kuleshevich <alexey.kuleshevich@iohk.io>
93f203d
to
ffc46e9
Compare
ffc46e9
to
b76dfb4
Compare
Description
This PR adds the
NonZero
module and gets rid of unsafe division (or at least makes the error explicit).closes #4696
Checklist
CHANGELOG.md
files updated for packages with externally visible changesNew section is never added with the code changes. (See RELEASING.md)
.cabal
andCHANGELOG.md
files when necessary, according to theversioning process.
.cabal
files updated when necessaryIf you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
scripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
updated (usescripts/gen-hie.sh
)