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

Generalize StrictT(M)Var invariant #1040

Merged
merged 1 commit into from
Sep 20, 2019
Merged

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Sep 18, 2019

Rather than just returning a Bool, we return Maybe an error message. This paves the way for more detailed information we get back from the NF check.

Marking this as a draft because still untested.

Rather than just returning a `Bool`, we return `Maybe` an error message.
This paves the way for more detailed information we get back from the NF
check.
@edsko edsko force-pushed the edsko/generalize-tvar-invariant branch from 5d58ef7 to 6dc6745 Compare September 18, 2019 12:38
@edsko
Copy link
Contributor Author

edsko commented Sep 18, 2019

Ok, this works. Ready for review and to be merged.

@edsko edsko marked this pull request as ready for review September 18, 2019 13:05
@edsko edsko requested a review from karknu September 18, 2019 13:06
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -18,6 +18,11 @@ source-repository head
location: https://github.com/input-output-hk/ouroboros-network
subdir: io-sim-classes

flag checktvarinvariant
Copy link
Contributor

Choose a reason for hiding this comment

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

When/where do we enable this flag?

Thinking out loud: we want to enable it always (?) for test-consensus and test-storage. We don't want to have to pass it manually via the cmd line.

Dealing with invariants
-------------------------------------------------------------------------------}

checkInvariant :: HasCallStack => Maybe String -> m x -> m x
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to reuse this function to check other invariants than this one? I mean instead of assert, which does not support a custom error message (or a call stack with a depth > 1).

Copy link
Contributor

@intricate intricate left a comment

Choose a reason for hiding this comment

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

💯

@edsko
Copy link
Contributor Author

edsko commented Sep 20, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Sep 20, 2019
1040: Generalize StrictT(M)Var invariant r=edsko a=edsko

Rather than just returning a `Bool`, we return `Maybe` an error message. This paves the way for more detailed information we get back from the NF check.

Marking this as a draft because still untested.

Co-authored-by: Edsko de Vries <[email protected]>
@edsko
Copy link
Contributor Author

edsko commented Sep 20, 2019

It's a good suggestion regarding enabling the flag on CI, but I don't know how to do it. Opened a separate ticket for it IntersectMBO/ouroboros-consensus#756 .

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 20, 2019

@iohk-bors iohk-bors bot merged commit 6dc6745 into master Sep 20, 2019
@iohk-bors iohk-bors bot deleted the edsko/generalize-tvar-invariant branch September 20, 2019 12:34
coot pushed a commit that referenced this pull request May 16, 2022
1040: Generalize StrictT(M)Var invariant r=edsko a=edsko

Rather than just returning a `Bool`, we return `Maybe` an error message. This paves the way for more detailed information we get back from the NF check.

Marking this as a draft because still untested.

Co-authored-by: Edsko de Vries <[email protected]>
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