Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-31321: Fix traceback.clear_frames()#3262

Closed
vstinner wants to merge 1 commit into
python:masterpython/cpython:masterfrom
vstinner:clear_framesCopy head branch name to clipboard
Closed

bpo-31321: Fix traceback.clear_frames()#3262
vstinner wants to merge 1 commit into
python:masterpython/cpython:masterfrom
vstinner:clear_framesCopy head branch name to clipboard

Conversation

@vstinner

@vstinner vstinner commented Sep 1, 2017

Copy link
Copy Markdown
Member

traceback.clear_frames() now iterates on frames to clear all frames
of each traceback object, not only the first frame.

https://bugs.python.org/issue31321

@vstinner

vstinner commented Sep 1, 2017

Copy link
Copy Markdown
Member Author

TODO: Convert script attached to http://bugs.python.org/issue31321 to an unit test.

Comment thread Lib/traceback.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making clear_frames() a potentially O(n**2) operation. In practice, you don't need this, as tb.tb_next.tb_frame.f_back will be the same as tb.tb_frame (and so on). So the loops needn't be nested.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you can keep the nesting but mark frames as seen as you iterate on them, which will have the same effect while being more general (in case someone changes the traceback chain).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making clear_frames() a potentially O(n**2) operation.

Sorry, I don't know well how traceback objects are created. I didn't even know that you can have two tracebacks objects linked together.

I added a "seen = set()" to prevent iterating twice on the same frame chain. Does it solve your O(n**2) issue?

I also used my example attached to the bpo to enhance the existing unit test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use while frame is not None here instead of having a separate test at the end of the loop.

traceback.clear_frames() now iterates on frames to clear all frames
of each traceback object, not only the first frame.

Enhance test_clear() unit test to check local variables of all
frames, not only the inner() frame.

Rename also the test to test_clear_frames() to better reflected te
name of the tested function.
@vstinner

Copy link
Copy Markdown
Member Author

I rebased my change and added a set to avoid O(n**2) complexity, as suggested by Antoine Pitrou (if I understood correctly his comment).

@pitrou: I didn't understand your second suggestion to avoid nested loop. If you find a way to avoid the nested loop without breaking my unit test, I'm curious :-)

@matrixise

Copy link
Copy Markdown
Member

Hi @vstinner and @pitrou what's the status of this PR?

@vstinner vstinner closed this Feb 1, 2018
@pitrou

pitrou commented Feb 1, 2018

Copy link
Copy Markdown
Member

@vstinner why did you close this PR? Is the fix obsolete?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.