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.

Sign up for GitHub

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

Jump to bottom

Fix for unconnected multiport and bank reactor bug #1953

Merged
merged 11 commits into from
Sep 12, 2023
Merged

Fix for unconnected multiport and bank reactor bug #1953

merged 11 commits into from
Sep 12, 2023

Conversation

OmerMajNition
Copy link
Collaborator

  • Unconnected multiport has compilation error, we needed an iterator which was missing
  • Bank reactor iterator was missing, causing bank reactor declaration failures

@OmerMajNition OmerMajNition added c Related to C target bugfix labels Aug 17, 2023
@OmerMajNition OmerMajNition self-assigned this Aug 17, 2023
@OmerMajNition OmerMajNition linked an issue Aug 17, 2023 that may be closed by this pull request
Unconnected Multiport output and Nested Bank reactor bug #1952
Closed
edwardalee
edwardalee reviewed Aug 17, 2023
View reviewed changes
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.

Can you provide tests for these?

@OmerMajNition
Copy link
Collaborator Author

@edwardalee, I've added test file for your review please.

@edwardalee
Copy link
Collaborator

@edwardalee, I've added test file for your review please.

The test seems overly complex. Is there a minimal case that illustrates the problem this was addressing? Such a case was missing in the original issue. Also, I don't think a test should be writing to an external log file.

@OmerMajNition
Copy link
Collaborator Author

OmerMajNition commented Aug 23, 2023 •
edited
Loading

@edwardalee, I've added few more test files. This compilation error occurs on unconnected output port.
For multi-port there's an error and in case of multi-bank there another error.

This PR fixes both errors.
Test cases pushed for your review please

@petervdonovan
Copy link
Collaborator

petervdonovan commented Aug 30, 2023 •
edited
Loading

I spoke to Marten about this but I will re-post here so we are all on the same page: Since this PR was delayed, I independently encountered and fixed the same problem while this PR was pending. This PR adds more test cases and may be a somewhat cleaner solution that what I did, so it might still be useful if anyone is interested in addressing the concerns raised and the merge conflicts.

lhstrh
lhstrh requested changes Sep 7, 2023
View reviewed changes
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Could you please remove the two files that are already passing in master and reduce the other two down to minimal examples (or one, if they both end up testing the same thing)? The change in the code generator looks OK to me...

@OmerMajNition OmerMajNition force-pushed the 1952-unconnected-multiport-output-and-nested-bank-reactor-bug branch from 81cea76 to d7514b8 Compare September 7, 2023 15:25
@OmerMajNition
Copy link
Collaborator Author

@lhstrh, removed the test case files that main is now already taking care of.
Please review again and let me know if anything else required from my end

@lhstrh lhstrh changed the title unconnected multi-port, and bank reactor bug Fix for unconnected multi-port and bank reactor bug Sep 7, 2023
@lhstrh lhstrh mentioned this pull request Sep 7, 2023
Closed
@lhstrh
Copy link
Member

lhstrh commented Sep 7, 2023

I'm seeing some mysterious errors on Windows happening in CI? Not sure what's going on there...

@OmerMajNition OmerMajNition force-pushed the 1952-unconnected-multiport-output-and-nested-bank-reactor-bug branch from c0afdea to c839b28 Compare September 8, 2023 10:18
lhstrh
lhstrh approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the fix, @OmerMajNition!

@lhstrh lhstrh enabled auto-merge September 11, 2023 06:44
lhstrh
lhstrh requested changes Sep 11, 2023
View reviewed changes
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Looks like the use of rand_r() is causing trouble on Windows. The tests should only include code that is strictly necessary to expose the bug that this PR is fixing...

@lhstrh lhstrh changed the title Fix for unconnected multi-port and bank reactor bug Fix for unconnected multiport and bank reactor bug Sep 11, 2023
@lhstrh lhstrh added this pull request to the merge queue Sep 11, 2023
Merged via the queue into master with commit 94c5b2c Sep 12, 2023
@lhstrh lhstrh deleted the 1952-unconnected-multiport-output-and-nested-bank-reactor-bug branch September 12, 2023 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwardalee edwardalee edwardalee left review comments

@lhstrh lhstrh lhstrh approved these changes

@petervdonovan petervdonovan Awaiting requested review from petervdonovan

Assignees

@OmerMajNition OmerMajNition

Labels
bugfix c Related to C target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unconnected Multiport output and Nested Bank reactor bug
4 participants
@OmerMajNition @edwardalee @petervdonovan @lhstrh