Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(317)

#27985: Implement PEP 526

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by guido
Modified:
1 year, 3 months ago
Reviewers:
brett, yselivanov, levkivskyi, storchaka+cpython
CC:
gvanrossum, devnull_psf.upfronthosting.co.za, Yury Selivanov, levkivskyi
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 91

Patch Set 3 #

Patch Set 4 #

Total comments: 6

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/glossary.rst View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
Doc/library/dis.rst View 1 2 3 4 2 chunks +11 lines, -0 lines 0 comments Download
Doc/reference/datamodel.rst View 1 2 3 4 2 chunks +21 lines, -14 lines 0 comments Download
Doc/reference/simple_stmts.rst View 1 2 3 4 2 chunks +44 lines, -0 lines 0 comments Download
Grammar/Grammar View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
Include/Python-ast.h View 1 2 3 4 3 chunks +16 lines, -5 lines 0 comments Download
Include/graminit.h View 1 2 3 4 1 chunk +69 lines, -68 lines 0 comments Download
Include/opcode.h View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
Include/symtable.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
Lib/importlib/_bootstrap_external.py View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
Lib/lib2to3/Grammar.txt View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
Lib/lib2to3/tests/test_parser.py View 1 2 3 4 1 chunk +30 lines, -0 lines 0 comments Download
Lib/opcode.py View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
Lib/symbol.py View 1 2 3 4 1 chunk +69 lines, -68 lines 0 comments Download
Lib/symtable.py View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
Lib/test/ann_module.py View 1 2 3 4 1 chunk +53 lines, -0 lines 0 comments Download
Lib/test/ann_module2.py View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
Lib/test/ann_module3.py View 1 2 3 4 1 chunk +18 lines, -0 lines 0 comments Download
Lib/test/pydoc_mod.py View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
Lib/test/test___all__.py View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
Lib/test/test_dis.py View 1 2 3 4 2 chunks +33 lines, -0 lines 0 comments Download
Lib/test/test_grammar.py View 1 2 3 4 3 chunks +178 lines, -0 lines 0 comments Download
Lib/test/test_opcodes.py View 1 2 3 4 2 chunks +27 lines, -0 lines 0 comments Download
Lib/test/test_parser.py View 1 2 3 4 1 chunk +39 lines, -0 lines 0 comments Download
Lib/test/test_pydoc.py View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
Lib/test/test_symtable.py View 1 2 3 4 1 chunk +11 lines, -0 lines 0 comments Download
Lib/test/test_tools/test_com2ann.py View 1 2 3 4 1 chunk +260 lines, -0 lines 0 comments Download
Lib/test/test_typing.py View 1 2 3 4 3 chunks +81 lines, -2 lines 0 comments Download
Lib/typing.py View 1 2 3 4 9 chunks +133 lines, -13 lines 0 comments Download
Modules/symtablemodule.c View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
PC/launcher.c View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
Parser/Python.asdl View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
Python/Python-ast.c View 1 2 3 4 7 chunks +123 lines, -1 line 0 comments Download
Python/ast.c View 1 2 3 4 3 chunks +84 lines, -2 lines 0 comments Download
Python/ceval.c View 1 2 3 4 2 chunks +112 lines, -0 lines 0 comments Download
Python/compile.c View 1 2 3 4 8 chunks +214 lines, -1 line 0 comments Download
Python/graminit.c View 1 2 3 4 20 chunks +1058 lines, -1031 lines 0 comments Download
Python/importlib_external.h View 1 2 3 4 99 chunks +104 lines, -104 lines 0 comments Download
Python/opcode_targets.h View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
Python/pylifecycle.c View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
Python/symtable.c View 1 2 3 4 3 chunks +56 lines, -0 lines 0 comments Download
Tools/parser/com2ann.py View 1 2 3 4 1 chunk +309 lines, -0 lines 0 comments Download
Tools/parser/unparse.py View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download

Messages

Total messages: 9
brett.cannon
Guido and I are parallel reviewing, so I worked bottom up through the Modules/ directory. ...
1 year, 3 months ago #1
gvanrossum
Here's my half of the review! Again mostly nits. See also something I put in ...
1 year, 3 months ago #2
Yury Selivanov
I only looked at C code -- looks solid. My only advice is to fix ...
1 year, 3 months ago #3
levkivskyi
Brett, Yury, thank you for review, I fill fix everything according to your comments (I ...
1 year, 3 months ago #4
levkivskyi
I hope I did not miss anything. Please take a look. -- Ivan http://bugs.python.org/review/27985/diff/18374/Lib/importlib/_bootstrap_external.py File ...
1 year, 3 months ago #5
storchaka
http://bugs.python.org/review/27985/diff/18374/Doc/reference/simple_stmts.rst File Doc/reference/simple_stmts.rst (right): http://bugs.python.org/review/27985/diff/18374/Doc/reference/simple_stmts.rst#newcode325 Doc/reference/simple_stmts.rst:325: Annotation assignment is the combination, in a single statement, ...
1 year, 3 months ago #6
levkivskyi
Serhiy, Thank you for review! I implemented your changes and will submit a clean patch ...
1 year, 3 months ago #7
gvanrossum
Looks like there are some promised fixes missing from this diff. Also it doesn't apply ...
1 year, 3 months ago #8
levkivskyi
1 year, 3 months ago #9
http://bugs.python.org/review/27985/diff/18374/Lib/typing.py
File Lib/typing.py (right):

http://bugs.python.org/review/27985/diff/18374/Lib/typing.py#newcode276
Lib/typing.py:276: if isinstance(t, TypingMeta) or isinstance(t, _ClassVar):
On 2016/09/08 22:27:09, gvanrossum wrote:
> On 2016/09/08 02:12:12, gvanrossum wrote:
> > Why is the second isinstance() needed? Isn't _ClassVar a subclass of
> TypingMeta
> > anyway?
> 
> Still unanswered.

We discussed this in tracker itself, ClassVar is not a class (for optimization
reasons I explained), therefore it is not an instance or Typing Meta

http://bugs.python.org/review/27985/diff/18374/Python/ceval.c
File Python/ceval.c (right):

http://bugs.python.org/review/27985/diff/18374/Python/ceval.c#newcode1879
Python/ceval.c:1879: PyObject *ann = TOP();
On 2016/09/08 22:27:09, gvanrossum wrote:
> On 2016/09/08 22:14:54, levkivskyi wrote:
> > On 2016/09/08 21:00:08, storchaka wrote:
> > > Just POP() instead of TOP() + STACKADJ(-1).
> > 
> > Done.
> 
> I don't see the fix yet?

This is in the new parch after resolving conflicts.

http://bugs.python.org/review/27985/diff/18374/Python/ceval.c#newcode1892
Python/ceval.c:1892: }
On 2016/09/08 22:27:09, gvanrossum wrote:
> On 2016/09/08 22:14:54, levkivskyi wrote:
> > On 2016/09/08 21:00:08, storchaka wrote:
> > > Following code can triggers executing Python code that can remove
> > > __annotations__ from locals. It is safer to incref borrowed reference
> > ann_dict.
> > 
> > I am not sure what do you mean here.
> > 
> > Do you mean PyObject_SetItem(ann_dict, name, ann) below can do this while
> > called? 
> > Anyway, I think you are right that it is safer. Fixed this by adding an
> INCREF.
> 
> Yes, PyObject_SetItem() could invoke Python code if there's a hash collision
> with a key that overrides comparison in Python.

This change is also in the new patch. I am running tests after resolving
conflicts, will submit new patch in few minutes.

http://bugs.python.org/review/27985/diff/18374/Python/ceval.c#newcode2729
Python/ceval.c:2729: if (PyDict_CheckExact(f->f_locals)) {
On 2016/09/08 22:27:09, gvanrossum wrote:
> On 2016/09/08 22:14:54, levkivskyi wrote:
> > On 2016/09/08 21:00:08, storchaka wrote:
> > > f_locals can be NULL.
> > 
> > Done.
> 
> Am I missing a new upload?

Yes, this is because I wanted to run tests after resolving conflicts, so there
is a slight delay with a new patch.

Although, no changes there apart from discussed here.

http://bugs.python.org/review/27985/diff/18374/Python/compile.c
File Python/compile.c (right):

http://bugs.python.org/review/27985/diff/18374/Python/compile.c#newcode207
Python/compile.c:207: static PyObject *__annotations__;
On 2016/09/08 22:27:09, gvanrossum wrote:
> On 2016/09/08 22:14:54, levkivskyi wrote:
> > On 2016/09/08 21:00:08, storchaka wrote:
> > > Names starting with double underscores are reserved by C compiler. Don't
use
> > > them.
> > > 
> > > It is better to use _Py_IDENTIFIER().
> > 
> > Thanks for info!
> > Fixed.
> 
> I don't see the fix yet.

In new patch. Just wait few minutes.

http://bugs.python.org/review/27985/diff/18414/Lib/test/test_grammar.py
File Lib/test/test_grammar.py (right):

http://bugs.python.org/review/27985/diff/18414/Lib/test/test_grammar.py#newco...
Lib/test/test_grammar.py:211: # execution oreder
On 2016/09/08 22:27:09, gvanrossum wrote:
> typo; oreder -> order

Done.

http://bugs.python.org/review/27985/diff/18414/Lib/test/test_grammar.py#newco...
Lib/test/test_grammar.py:213: no_name[fff()]: no_name_again = 1/0
On 2016/09/08 22:27:09, gvanrossum wrote:
> Looks like fff() really stands for any expression that raises NameError --
maybe
> change this to does_not_exist? (Don't call it either.)

Done.

http://bugs.python.org/review/27985/diff/18414/Lib/test/test_grammar.py#newco...
Lib/test/test_grammar.py:319: # comlex case: custom locals plus custom
__annotations__
On 2016/09/08 22:27:09, gvanrossum wrote:
> typo: comlex -> complex

Done.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7