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

cleanup: extra_args and argidx #4311

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

widlarizer
Copy link
Collaborator

This PR simplifies and unifies boilerplate by removing unused iteration over pass args which have been left in as placeholders. It also removes commented out arg checks that I think aren't relevant to the pass at hand and have only been copied from another pass.

Incidentally, by setting argidx correctly it also prevents the warning opt_demorgan would unconditionally throw: Warning: Selection "opt_demorgan" did not match any module.

@povik
Copy link
Member

povik commented Apr 2, 2024

I am not sure removing the argument iteration boilerplate is desirable, I am slightly inclined against it, to be frank. The command-line interface to those passes could get expanded at any point.

@whitequark
Copy link
Member

Incidentally, by setting argidx correctly it also prevents the warning opt_demorgan would unconditionally throw: Warning: Selection "opt_demorgan" did not match any module.

This seems like a bug fix that should be submitted separately.

@widlarizer
Copy link
Collaborator Author

@whitequark true, opened #4313

@povik It can be expanded at any point by copying from another pass. Or we can make an example pass to make contributing new passes easier. It doesn't seem necessary at all to have this boilerplate in some passes and not others, without explicit TODOs about what should be added. I mean it's a cosmetic change, but I think letting unused code and variables hang around can be distracting

@povik
Copy link
Member

povik commented Apr 3, 2024

Right, it's a cosmetic thing and a matter of taste. Let's leave it up to the dev JF, and if my position will be in the minority let's merge it.

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.

None yet

3 participants