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

Bugfix for when top-level multiport width in federation depends on parameter #1956

Merged
merged 7 commits into from Aug 23, 2023

Conversation

petervdonovan
Copy link
Collaborator

@petervdonovan petervdonovan commented Aug 17, 2023

This simply computes the concrete numeric value of the parameter at compile time.

Closes #1942.

Closes #1957.

Closes #1961.

Instead of adding a test, this PR simply modifies an existing test. The disadvantage of this approach is that when one test is used to test many features, it may not be as useful as a minimal reproducible example. The advantage of this approach is that it does not significantly increase the time/cost of running all the tests, whereas adding a new test creates redundant work by executing code that is common to all the tests.

@petervdonovan
Copy link
Collaborator Author

This PR uncovered #1957 by making some ports into multiports that were not previously multiports (because we went from no width spec to a width spec of [1]). This is why the PR is currently delayed.

@petervdonovan petervdonovan marked this pull request as draft August 17, 2023 22:04
@petervdonovan petervdonovan marked this pull request as ready for review August 17, 2023 22:24
@petervdonovan
Copy link
Collaborator Author

I'm not sure what is going on with the TS federated tests, so I re-ran them. I will need to take another look before this can get merged.

Copy link
Collaborator

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@edwardalee
Copy link
Collaborator

It looks like this also fixes #1940

However, it exposes a new bug: #1961

@petervdonovan
Copy link
Collaborator Author

I think I fixed the problem with the failing TypeScript test as well as #1961. Let's see if the tests pass.

@petervdonovan petervdonovan added this pull request to the merge queue Aug 23, 2023
Merged via the queue into master with commit 8505fb5 Aug 23, 2023
40 checks passed
@petervdonovan petervdonovan deleted the fix-fed-multiport-params branch August 23, 2023 07:45
@lhstrh lhstrh changed the title Fix bug when top-level multiport width in federation depends on parameter Bugfix for when top-level multiport width in federation depends on parameter Aug 28, 2023
@lhstrh lhstrh added the bugfix label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants