Skip to content
This repository has been archived by the owner on May 31, 2020. It is now read-only.

with statement #594

Merged
merged 1 commit into from
Aug 7, 2017
Merged

with statement #594

merged 1 commit into from
Aug 7, 2017

Conversation

abonie
Copy link
Contributor

@abonie abonie commented Jul 31, 2017

Implement SETUP_WITH and WITH_CLEANUP opcodes in Batavia's VM

this.push_block(block.type, block.handler, block.level-1)
} else {
throw new builtins.BataviaError.$pyclass('Confused WITH_CLEANUP')
}

Choose a reason for hiding this comment

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

throw new builtins.BataviaError.$pyclass('Confused WITH_CLEANUP')
}
var ret = callables.call_function(exit_func, [exc, val, tb]) // XXX this arg list is incorrect for first 2 cases
//TODO: silence error if necessary

Choose a reason for hiding this comment

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

this.push('silenced')
}
}

Choose a reason for hiding this comment

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

this.push_block(block.type, block.handler, block.level - 1)
} else {
throw new builtins.BataviaError.$pyclass('Confused WITH_CLEANUP')
}

Choose a reason for hiding this comment

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

this.push_block(block.type, block.handler, block.level - 1)
} else {
throw new builtins.BataviaError.$pyclass('Confused WITH_CLEANUP')
}

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what's going on here - the lint problem is genuine; but it's on line 1747.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it. The XXX was only there to remind me to double check that this line works as intended. But now I am wondering if I should leave it as is, since it breaks "stack API" by using this.frame.stack.slice instead of combination of this.pop and this.push. The other way feels clunky in this case.

// this.push(ctxmgr_obj)
// }
VirtualMachine.prototype.byte_SETUP_WITH = function(dest) {
var mgr = this.top()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CPython would pop context manager from the stack here and push only __exit__ method of the context manager to call it as a function in WITH_CLEANUP. However I couldn't make it work this way, so I am leaving context manager on the stack and in WITH_CLEANUP I am calling mgr.__exit__ (line 1753). I don't see how this would break anything. What do you think @freakboy3742 ?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. The state of the stack is an internal detail; and in this particular case, it's a transient detail - so go right ahead.

@abonie abonie changed the title [WIP] with statement with statement Aug 2, 2017
Opcodes SETUP_WITH and WITH_CLEANUP has been implemented to trigger
context manager's `__enter__` and `__exit__` methods as necessary and
suppress exceptions when appropriate.

Note that Python 3.5+ replaces WITH_CLEANUP with WITH_CLEANUP_START and
WITH_CLEANUP_FINISH.
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

with approve():
    looks_great()

@freakboy3742 freakboy3742 merged commit 20185c2 into beeware:master Aug 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants