Skip to content

Introduce invariant check.#63

Merged
vrom911 merged 1 commit intokowainik:masterfrom
qnikst:invariant-check
Aug 18, 2019
Merged

Introduce invariant check.#63
vrom911 merged 1 commit intokowainik:masterfrom
qnikst:invariant-check

Conversation

@qnikst
Copy link
Copy Markdown
Contributor

@qnikst qnikst commented Oct 18, 2018

This check provides if internal structure maintains required
invariants and can be used when changing the logic of the
modification functions.

@vrom911 vrom911 added Hacktoberfest https://hacktoberfest.digitalocean.com/ enhancement tests labels Oct 18, 2018
Comment thread test/Test/TypeRep/MapProperty.hs Outdated
test_DeleteInvariant = prop "invariantCheck (delete k b) == True" $ do
m <- forAll genMap
WrapTypeable (proxy :: IntProxy n) <- forAll genTF
invariantCheck (delete @n m) === True
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to use assert from the hedgehog library instead of === True

-- Helper functions.
----------------------------------------------------------------------------

invariantCheck :: TypeRepMap f -> Bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you, please, add documentation to what this invariant actually checks? It's not clear to me what is the invariant of TypeRepMap

Comment thread src/Data/TypeRepMap/Internal.hs Outdated

invariantCheck :: TypeRepMap f -> Bool
invariantCheck TypeRepMap{..} = getAll (check 0) where
sz = sizeofPrimArray fingerprintAs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefer to write where on the separate line and add two spaces to the function definition and also add types to the functions inside where. This makes code more readable, and this helps a lot when trying to understand difficult and complicated code. Like this:

invariantCheck TypeRepMap{..} = getAll (check 0)
  where
    sz :: ???
    sz = sizeofPrimArray fingerprintAs
    
    check :: ???
    check = ...

@qnikst
Copy link
Copy Markdown
Contributor Author

qnikst commented Oct 19, 2018

I have addressed all the comments, please take another look.

Copy link
Copy Markdown
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

I can approve and merge this PR to the master branch. But, strictly speaking, the checked invariant is not strong. The TypeRepMap structure satisfies the more strong invariant. Basically, array represents binary search tree. Consider the following array:

a[0] = 5
a[1] = 3  -- left son of a[0]
a[2] = 10 -- right son of a[0]
a[3] = 0  -- left son of a[1]
a[4] = 7  -- right son of a[1]

You can see that this array satisfies invariant you check, but this is a wrong binary search tree, because left branch of node with value 5 contains element that is greater than 5.

@qnikst
Copy link
Copy Markdown
Contributor Author

qnikst commented Oct 20, 2018

yep, let me update the check

@qnikst
Copy link
Copy Markdown
Contributor Author

qnikst commented Jan 24, 2019

self reminder (to pop up in email)

@chshersh
Copy link
Copy Markdown
Contributor

@qnikst Friendly ping 🙂

This check provides if internal structure maintains required
invariants and can be used when changing the logic of the
modification functions.
@qnikst
Copy link
Copy Markdown
Contributor Author

qnikst commented Aug 17, 2019

Seems I was a bit busy, sorry for delay. I've rebased my branch atop of the master and addressed the comments.

Copy link
Copy Markdown
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Thanks! This check looks useful!

Copy link
Copy Markdown
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Thank you!

@vrom911 vrom911 merged commit 2aff258 into kowainik:master Aug 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Hacktoberfest https://hacktoberfest.digitalocean.com/ tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants