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

#25843: lambdas on the same line may incorrectly share code objects

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 3 months ago by tijs.vanoevelen
Modified:
4 years, 2 months ago
Reviewers:
storchaka, victor.stinner
CC:
arigo, rhettinger, mark.dickinson, Nick Coghlan, haypo, larry, cartman, ezio.melotti, r.david.murray, torsten, fijall, devnull_psf.upfronthosting.co.za, storchaka, david.maciver_gmail.com, ebarry, Kevin Shweh, tijs.vanoevelen_gmail.com
Visibility:
Public.

Patch Set 1 #

Total comments: 4

Patch Set 2 #

Total comments: 4

Patch Set 3 #

Total comments: 10

Patch Set 4 #

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Include/code.h View 1 2 3 4 1 chunk +10 lines, -1 line 0 comments Download
Lib/test/test_compile.py View 1 2 3 4 1 chunk +82 lines, -0 lines 0 comments Download
Objects/codeobject.c View 1 2 3 4 2 chunks +138 lines, -1 line 0 comments Download
Python/compile.c View 1 2 3 4 6 chunks +10 lines, -48 lines 0 comments Download

Messages

Total messages: 4
storchaka_gmail.com
http://bugs.python.org/review/25843/diff/16155/Objects/codeobject.c File Objects/codeobject.c (right): http://bugs.python.org/review/25843/diff/16155/Objects/codeobject.c#newcode419 Objects/codeobject.c:419: if (PyFloat_Check(o)) { May be use PyFloat_CheckExact? http://bugs.python.org/review/25843/diff/16155/Objects/codeobject.c#newcode441 Objects/codeobject.c:441: ...
4 years, 3 months ago #1
storchaka_gmail.com
http://bugs.python.org/review/25843/diff/16156/Objects/codeobject.c File Objects/codeobject.c (right): http://bugs.python.org/review/25843/diff/16156/Objects/codeobject.c#newcode502 Objects/codeobject.c:502: /* set size changed during iteration, ignore */ Is ...
4 years, 3 months ago #2
storchaka_gmail.com
http://bugs.python.org/review/25843/diff/16394/Include/code.h File Include/code.h (right): http://bugs.python.org/review/25843/diff/16394/Include/code.h#newcode114 Include/code.h:114: #ifndef Py_LIMITED_API Would be better to move #ifndef Py_LIMITED_API ...
4 years, 2 months ago #3
victor.stinner_gmail.com
4 years, 2 months ago #4
http://bugs.python.org/review/25843/diff/16155/Objects/codeobject.c
File Objects/codeobject.c (right):

http://bugs.python.org/review/25843/diff/16155/Objects/codeobject.c#newcode419
Objects/codeobject.c:419: if (PyFloat_Check(o)) {
On 2015/12/14 18:23:18, storchaka wrote:
> May be use PyFloat_CheckExact?

I only moved the code, I didn't write this part myself.

But ok, I changed it to CheckExact.

http://bugs.python.org/review/25843/diff/16155/Objects/codeobject.c#newcode441
Objects/codeobject.c:441: Py_None, Py_None, Py_None);
On 2015/12/14 18:23:18, storchaka wrote:
> May be use different tags None, True, False, nothing?

Ok, I used your suggestion.

http://bugs.python.org/review/25843/diff/16156/Objects/codeobject.c
File Objects/codeobject.c (right):

http://bugs.python.org/review/25843/diff/16156/Objects/codeobject.c#newcode502
Objects/codeobject.c:502: /* set size changed during iteration, ignore */
On 2015/12/14 20:29:30, storchaka wrote:
> Is it ever possible? If yes, we have more serious issue not holding an
ownership
> of proceeded objects. But I think that this is impossible.

Ok, I replaced it with an assertion.

http://bugs.python.org/review/25843/diff/16156/Objects/codeobject.c#newcode506
Objects/codeobject.c:506: return keys;
On 2015/12/14 20:29:30, storchaka wrote:
> Just keys? Not wrap it in a tuple?

Ok, I will wrap the set inside a tuple, as other types :-)

> And note that keys is ordered while original set is not.

Oh right, it will recreate a new set/frozenset.

http://bugs.python.org/review/25843/diff/16394/Lib/test/test_compile.py
File Lib/test/test_compile.py (right):

http://bugs.python.org/review/25843/diff/16394/Lib/test/test_compile.py#newco...
Lib/test/test_compile.py:633: self.check_constant(f2, const2)
On 2016/01/20 19:19:01, storchaka wrote:
> Assert repr(f1()) == repr(const1) and  repr(f2()) == repr(const2)?

Good idea, thanks for the hint.

http://bugs.python.org/review/25843/diff/16394/Lib/test/test_compile.py#newco...
Lib/test/test_compile.py:636: check_different_constants(+0.0, -0.0)
On 2016/01/20 19:19:01, storchaka wrote:
> In Python 3 this test is passed because co_consts contains both 0.0 and -0.0
for
> -0.0, but only 0.0 for 0.0. It is worth to add a test with equal number of
> constants and failed with current code. May something like (if there is no
> simpler example):
> 
>     a, b = lambda: [0.0, -0.0, (-0.0, 0.0)], lambda: [0.0, -0.0, (0.0, -0.0,)]

IMHO it's overkill to check the exact list of constants. The unit test is
already complex enough for a corner case. Other tests are enough to detect bugs.

But yeah, I'm aware of the implementation issue of the peephole optimizer.
Sign in to reply to this message.

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