-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] Support __setitem__, __delitem__, __len__ and __contains__ #10451
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
Conversation
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.
Looks good! One minor point that doesn't block merging.
generated = {} # type: Dict[str, str] | ||
# Sort for determinism on Python 3.5 | ||
for name, (slot, generator) in sorted(table.items()): | ||
for name, (slot, generator) in sorted(table.items(), reverse=True): |
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.
why turn on reverse?
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.
This because the implementation depends on processing __setitem__
before __delitem__
. This kind of hacky -- I'll add a comment in a follow-up PR, or see if this could be refactored.
Related issue: mypyc/mypyc#839 |
Previously these couldn't be used in compiled classes.
__setitem__
and__delitem__
are implemented by a single wrapper function. If the value isNULL, it should act as
__delitem__
. This makes the implementation non-trivial. First, weneed to support both a single variant and two variants being defined in a class. Second, when
overriding, we need to use
super()
, if we only override one of them.The other dunders are straightforward.
The implementation is pretty verbose. I'll look at refactoring it in a separate PR.
This has some overlap with the previous PR #10211 by @sohailsomani.