-
Notifications
You must be signed in to change notification settings - Fork 154
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
Keep casts of the form x ~ Integer
#1928
Conversation
Really a work-around until we properly handle all casts. Fixes #1927
@@ -659,6 +659,8 @@ isSizedCast (TyConApp tc1 _) (TyConApp tc2 _) = do | |||
,tc2 `hasKey` naturalTyConKey && | |||
tc1Nm == "Clash.Sized.Internal.Unsigned.Unsigned" | |||
]) | |||
-- XXX: a work-around for issue #1927, the real fix is to handle all casts | |||
isSizedCast (TyVarTy {}) (TyConApp tc2 _) = return (tc2 `hasKey` integerTyConKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also need a case for Integer ~ x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for issue #1927 specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but would you trigger the same issue by changing counter ~ (Index numStages)
to (Index numStages) ~ counter
in the test? If so I think we should add the extra case to avoid weird (albeit unlikely) situations where someone's design fails unexpectedly unless they change the argument order to (~)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried. I'm somewhat uncomfortable with this PR to begin with. I'd rather properly support all casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, supporting all casts would be the better option. How far was #1064 from being finished?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It went in the right direction. But I would want to redo some aspects, especially with regards to Tick
. So squashCollectApp
was there in terms of handling casts I think, but Ticks
still need more care: depending on the type of info (source location, HDL naming, etc.) the ticks either should or shouldn't be distributed towards the arguments as we "squash" the entire application.
Superseded by #1064 |
Really a work-around until we properly handle all casts.
Fixes #1927