Skip to content

Always output fully-simplified states #2303

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

Merged
merged 7 commits into from
Dec 9, 2020
Merged

Always output fully-simplified states #2303

merged 7 commits into from
Dec 9, 2020

Conversation

emarzion
Copy link
Contributor

@emarzion emarzion commented Dec 8, 2020

Fixes #2299

Update executionStrategy so that Simplify always appears after Rewrite. If execution is stopped by the depth limit, fully-simplified states will be printed for the user.


Review checklist

The author performs the actions on the checklist. The reviewer evaluates the work and checks the boxes as they are completed.

  • Summary. Write a summary of the changes. Explain what you did to fix the issue, and why you did it. Present the changes in a logical order. Instead of writing a summary in the pull request, you may push a clean Git history.
  • Documentation. Write documentation for new functions. Update documentation for functions that changed, or complete documentation where it is missing.
  • Tests. Write unit tests for every change. Write the unit tests that were missing before the changes. Include any examples from the reported issue as integration tests.
  • Clean up. The changes are already clean. Clean up anything near the changes that you noticed while working. This does not mean only spatially near the changes, but logically near: any code that interacts with the changes!

@ttuegel ttuegel self-requested a review December 8, 2020 15:11
@ttuegel ttuegel changed the title changing order produced by executionStrategy and adding unit tests Always output fully-simplified states Dec 8, 2020
@@ -119,7 +126,7 @@ data Prim
= Begin
| Simplify
| Rewrite
deriving (Show)
deriving (Eq,Show)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
deriving (Eq,Show)
deriving (Eq, Show)

, Rewrite
, Simplify
]
& Stream.iterate id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using where would follow the style guide a bit better:

    step1 Stream.:> Stream.iterate id stepN
  where
    step1 =
        (Strategy.sequence . fmap Strategy.apply)
            [ Begin
            , Simplify
            , Rewrite
            , Simplify
            ]
    stepN =
        (Strategy.sequence . fmap Strategy.apply)
            [ Begin
            , Rewrite
            , Simplify
            ]

Comment on lines 318 to 323
getPrims :: Strategy Prim -> [Prim]
getPrims = \case
Strategy.Seq s1 s2 -> getPrims s1 ++ getPrims s2
Strategy.Apply p -> [p]
Strategy.Continue -> []
_ -> [] -- ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's write a function isLastSimplify :: Strategy Prim -> Bool in the same spirit as hasRewrite, so that we can cover all the cases better.

Copy link
Contributor

@ttuegel ttuegel left a comment

Choose a reason for hiding this comment

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

Please also have a look at our style guide.

@emarzion emarzion requested a review from ttuegel December 9, 2020 07:12
Copy link
Contributor

@ttuegel ttuegel left a comment

Choose a reason for hiding this comment

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

I wrote a summary in the pull request this time, but please do that on your next pull request.

@ttuegel ttuegel merged commit 2e1a306 into master Dec 9, 2020
@ttuegel ttuegel deleted the fully-simplified branch December 9, 2020 20:51
ttuegel added a commit that referenced this pull request Dec 10, 2020
* executionStrategy: Simplify always appears after Rewrite

* limitedExecutionStrategy: Add missing documentation

Co-authored-by: Thomas Tuegel <[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.

Always output fully-simplified states
2 participants