-
Notifications
You must be signed in to change notification settings - Fork 745
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
Periodic maintenance PR #1376
Periodic maintenance PR #1376
Conversation
from typing import Union | ||
|
||
|
||
class X86_CPU_MODEL(Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to sync this automatically from Unicorn? Or is it blocked by #1629?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is unrelated.
I did consider building the enumeration in runtime through reflection / introspection but that would miss the point of simplifying the usage by enabling IDE hints: When the enumeration is available in editing time the IDE can assist with autocompletion and linting. If the enumeration is built in runtime, the IDE doesn't know what to expect there.
I agree that enumerating everything in runtime is more maintainable and elegant (?), but I think usability comes first. See a past PR with a similar idea that got rejected: #581
I think that the way it is now, backed up by tests, should cover the advantages of both methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would miss the point of simplifying the usage by enabling IDE hints
This can be done by adding the interfaces in unicorn. Like
def cpus() -> [CPU1|CPU2|CPU3]
# auto generated enums
return ....
I think that the way it is now, backed up by tests, should cover the advantages of both methods.
I agree. I'm also fine with this approach anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a toally different story.
Unicorn doesn't use Enums, only integer values, and could improve a lot if it goes that path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a toally different story. Unicorn doesn't use Enums, only integer values, and could improve a lot if it goes that path.
Cool, I will try to do that in 2.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I planned on doing that at some point, but first I wanted to make sure I am not doing that for nothing. The original PR is pending for more than a year now..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for long delay as too many things happened last year. I promise that PR will go to 2.1.0 as I’m trying to work on it.
Yet another maintenance PR including bug fixes and various improvements.
Highlights:
New Features: