-
Notifications
You must be signed in to change notification settings - Fork 111
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
Position and signed enums #244
base: master
Are you sure you want to change the base?
Conversation
This is an attempt to correct the Position enum values, see Parser.FIXME_enums and generate signed and unsigned enums, see PythonGenerator.generate_enums.
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.
Nice work. I have some naming issues:
- the "FIXME" is by convention a marker for code needing work. I cannot quite understand from the current code if it used in this way here (indicating that we should definitely find a better fix). In all cases, I would rather name the variable as hardfixed_enums for instances, and possibly add a # FIXME comment to indicate there is more work needed.
- the Enum/Enim is nice, but takes a while to understand. Would it not be better to use explicitly UnsignedEnum / SignedEnum?
One last thing: the hardcoded value for libvlc_position_top seems wrong to me. AFAIK, it should be 3, not 4 ? And 6 for libvlc_position_bottom, no?
For my last comment (about enum values) - my remembering of the C spec is correct, running the following code
indeed returns
|
Please, do use better names instead of the current, acronymic ones. The top value is 4 in the screenshot in issue #243, the documentation also shows top=4. Using 4 in |
For your consideration, attached is the Also, all 9 The |
This is an attempt to correct (a) the
Position
enum values, seeParser.FIXME_enums
and (b) generate signed and unsigned enums, seePythonGenerator.generate_enums
from issue #243.