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

Fixes #132: Nontermination when using datatypeNameModifier with recursive data types #133

Merged

Conversation

amiddelk
Copy link

The original code seems to check whether the type is really a
type constructor thus starting with an upper case letter.
The problem is that this check is done on the transformed name
of the type, which may no longer be an upper case letter
(e.g. when one uses camelTo2 '_'). Therefore, the check should
be done on the original name of the type, which then fixes
issue #132.

The original code seems to check whether the type is really a
type constructor thus starting with an upper case letter.
The problem is that this check is done on the transformed name
of the type, which may no longer be an upper case letter
(e.g. when one uses camelTo2 '_'). Therefore, the check should
be done on the original name of the type, which then fixes
issue GetShopTV#132.
@phadej
Copy link
Collaborator

phadej commented Oct 24, 2017

Could you add an regression test?

Without the fix of GetShopTV#132, the added testcase causes nontermination.
With the fix, the tests are all green.
@amiddelk
Copy link
Author

I've added a variant of the MyRoseTree testcases. It should cover the problem and fix pretty well.

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

LGTM

@fizruk
Copy link
Member

fizruk commented Oct 25, 2017

Nice fix! Thanks @amiddelk 👍

@fizruk fizruk merged commit c669a64 into GetShopTV:master Dec 31, 2017
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