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
|
@serhiy-storchaka can you remove the first commit, I think it is unrelated to this bug. |
|
This PR depends on #19037. After merging #19037 and synchronizing this PR with master the final diff will look cleaner. For now you can choose what commits you want to see in https://github.com/python/cpython/pull/19038/files. |
Co-Authored-By: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>
| case AugLoad: | ||
| case AugStore: | ||
| break; | ||
| default: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Could we keep the default just in case we receive some invalid value in the switch? (The same applies with the other ones). I may be missing the reason why you remove them, tough 🤔
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is impossible to get invalid value in ctx. compiler_nameop() is called either with a literal value or with e->v.Name.ctx, and it is validated before setting to e->v.Name.ctx.
It is more robust to not add the default because the compiler will warn if we add a new member to the expr_context_ty enum but do not have a corresponding case in a switch. If keep the default, the compiler would not catch this error at compile time.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks for the explanation!
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks Serhiy, this is a great improvement!
| VISIT(c, expr, e->v.Attribute.value); | ||
| ADDOP(c, DUP_TOP); | ||
| ADDOP_NAME(c, LOAD_ATTR, e->v.Attribute.attr, names); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nice! I find this code much cleaner and straightforward :)
|
|
||
| d_bytecode = (a->a_offset - a->a_lineno_off) * sizeof(_Py_CODEUNIT); | ||
| d_lineno = i->i_lineno - a->a_lineno; | ||
| if (d_lineno == 0) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nit: Pep 7 (Ignore if you want, I think this is also in the other PR)
https://bugs.python.org/issue39988