Skip to content

Refactor full_selection (again) #5000

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

Merged
merged 9 commits into from
Apr 10, 2025
Merged

Conversation

KrystalDelusion
Copy link
Member

@KrystalDelusion KrystalDelusion commented Apr 8, 2025

What are the reasons/motivation for this change?

#4768 had some bugs that were missed prior to merging, and with the imminent release of v0.52 it was reverted instead of trying to fix on main (which #4996 started doing, but was still getting failures in CI).

Explain how this is achieved.

If applicable, please suggest to reviewers how they can test the change.

Revert the reversion so that we can fix the bugs that the PR missed.
i.e. making explicit the ones that aren't intended for direct use.
Developer facing, intended to check internal selection semantics work as expected.  i.e. it would have revealed the bug in the now reverted PR.
Also test they work as expected.
Use `Design::selected_modules()` directly, popping at the end instead of copying the selection.
Also default to a complete selection so that boxes work as before.
Simplify to using `RTLIL::SELECT_WHOLE_CMDERR` instead of doing it manually.
Also add tests for importing selections with boxes.
The two test scripts affected use boxed modules directly; under normal usage the warning shouldn't appear.
@KrystalDelusion KrystalDelusion marked this pull request as ready for review April 8, 2025 05:08
@nakengelhardt nakengelhardt merged commit 3410e10 into main Apr 10, 2025
43 checks passed
@nakengelhardt nakengelhardt deleted the krys/re_refactor_selections branch April 10, 2025 16:06
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.

2 participants