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

Minor simplification of Python boolean #305

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Oct 27, 2024

No description provided.

Copy link
Member

@IIITM-Jay IIITM-Jay left a comment

Choose a reason for hiding this comment

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

@Timmmm I have left some comments as to why we are following the existing approach for better readability.

include_pseudo = False
if "-pseudo" in sys.argv[1:]:
include_pseudo = True
include_pseudo = "-pseudo" in sys.argv[1:]
Copy link
Member

Choose a reason for hiding this comment

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

I agree to the concise approach being followed here to reduce the number of lines of codes. Given that we should focus on optimizing the codes as much as possible, but at the same time, we should not forget its maintainability and readabaility.

Following, "Explicit is better than Implicit": the core tenet of Python, the already existing codes explicitly communicates the default state for the inclusion of pseudo instructions as (False) and only sets it to True conditionally, making the flow of logic clearer. Readers can follow the intent more easily than inferring the meaning from a single line assignment.

Additionally, I am tagging the discussions for reference regarding pseudo instructions inclusion for more information as to why we are using flags for explicit behaviour : #286 and #289

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't say I agree here. This isn't more explicit; it's just more verbose.

This is standard best practice. See this blog for example.

In the long term it would be much better to use argparse of course, but this is a pretty straightforward simplification.

Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

I agree with @Timmmm on this one. Since it isn't an esoteric construction, the conciseness improves readability.

@aswaterman aswaterman merged commit aa56879 into riscv:master Oct 28, 2024
2 checks passed
@Timmmm Timmmm deleted the bool_simplification branch October 29, 2024 21:11
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.

3 participants