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

Use Clash length of type instead of VHDL length inference. Fix #373 #376

Merged
merged 2 commits into from
Oct 11, 2018

Conversation

martijnbastiaan
Copy link
Member

Please see this issue.

While all test cases perfectly pass, I wonder if this patch is actually correct:

  1. Should we replace ALL such 'range constructs with just literals?
  2. Is the ~IF ~VIVADO construct still valid after this patch?

@martijnbastiaan martijnbastiaan force-pushed the hotfix-issue-373 branch 2 times, most recently from 9193e51 to 0889bb6 Compare October 11, 2018 07:18
Referencing `'length` and `'range' might cause problems when dealing
with 'empty' vectors, such as a vector of units. Thus, this commit
removes references to it, except in cases where it is used on ~RESULT,
as we assume that a function result is not empty (as it should have been
removed by Clash). This commit only affects VHDL, as we do not use
these constructs for Verilog.
@martijnbastiaan
Copy link
Member Author

I've removed a number of other references to 'length and 'range too as a precaution. This should generate effectively the same code, albeit a bit uglier. Can I interpret your 'approved' as a 'yes' to my questions too @christiaanb ?

@christiaanb
Copy link
Member

@martijnbastiaan the approved is a yes to your question, the If Vivado stuff should still work.

@martijnbastiaan
Copy link
Member Author

Perfect, thanks, I'll merge it soon.

@martijnbastiaan martijnbastiaan self-assigned this Oct 11, 2018
@martijnbastiaan martijnbastiaan merged commit 7e88a1d into master Oct 11, 2018
@martijnbastiaan martijnbastiaan deleted the hotfix-issue-373 branch October 11, 2018 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants