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

Improve handling of ArrayFlags #9334

Merged
merged 1 commit into from
Apr 18, 2023
Merged

Improve handling of ArrayFlags #9334

merged 1 commit into from
Apr 18, 2023

Conversation

mikeshardmind
Copy link
Contributor

Summary

This is a micro-optimization. Figured since it was easy to improve on, and these need to be created for potentially a lot of events, it didn't hurt to do slightly better.

Why Discord did it this way instead of as an integer bitfield like everything reasonable is unknowable, but this does less work to get to the same result.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

I'd merge this but it's obscenely unreadable compared to the original code. It could use a small comment to clarify it for future readers.

Why discord did it this way instead of as an integer bitfield like everything reasonable is unknowable, but this does less work to get to the same result.

Add comment detailing what many would find unreadbale
@mikeshardmind
Copy link
Contributor Author

I've added an in-depth comment on this. I don't know why Discord went this route with it, but better to make sure anyone reading it later has an explanation, some of what's done there would be obscure to future readers.

Copy link
Owner

@Rapptz Rapptz left a comment

Choose a reason for hiding this comment

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

Thanks.

@Rapptz Rapptz merged commit 8991525 into Rapptz:master Apr 18, 2023
@mikeshardmind mikeshardmind deleted the patch-1 branch April 18, 2023 08: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