Skip to content

Conversation

@ana-pantilie
Copy link
Contributor


Reviewer checklist
  • Test coverage: stack test --coverage
  • Public API documentation: stack haddock

@ana-pantilie ana-pantilie marked this pull request as draft November 11, 2020 11:38
@ana-pantilie ana-pantilie marked this pull request as ready for review November 12, 2020 17:13
@ana-pantilie ana-pantilie requested a review from ttuegel November 12, 2020 17:14
ApplySymbolF app
| isFunctionalPattern pat ->
translateApplication app
<|> translateUninterpreted'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we add this to translateApplication itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggesting this, I made the change and added some comments on the PR about the additional things I had to modify resulting from this.

Comment on lines -247 to -248
sexpr <- maybe empty return translated
when (isNothing translated)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, if translated is Nothing this gets discarded before logging the warning. My changes should fix this.

Comment on lines +243 to +247
| isFunctionalPattern original =
translateInterpretedApplication
<|> translateUninterpreted'
| otherwise =
translateInterpretedApplication
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, we would fall back to translateUninterpreted for translateInt and translateBool without first checking if the pattern is functional. I assumed this wasn't correct, and fixed it here. This is reflected in the tests by the need to add some test data with the functional attribute.

@ana-pantilie ana-pantilie requested a review from ttuegel November 13, 2020 13:25
@ana-pantilie ana-pantilie merged commit 8efd16c into runtimeverification:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants