Skip to content

gh-111354: remove comparisons with enum values, variable reuse, unused imports in genobject.c #111708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 9, 2023
Merged

gh-111354: remove comparisons with enum values, variable reuse, unused imports in genobject.c #111708

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
367f548
inline exc_state_traverse
iritkatriel Nov 3, 2023
470f82e
remove comparisons with frame states
iritkatriel Nov 3, 2023
3dc7586
variable reused for two different purposes
iritkatriel Nov 3, 2023
677fc3e
remove unused #includes
iritkatriel Nov 3, 2023
0936160
add RESUME_WITH_SUBITERATOR macro
iritkatriel Nov 3, 2023
e7a4b70
Merge branch 'main' into genobject
iritkatriel Nov 6, 2023
8bba3dc
Merge branch 'main' into genobject
iritkatriel Nov 6, 2023
6bd958c
Revert "add RESUME_WITH_SUBITERATOR macro"
iritkatriel Nov 6, 2023
1f30519
FRAME_STATE_CLOSED --> FRAME_STATE_FINISHED
iritkatriel Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Include/internal/pycore_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ typedef enum _framestate {
} PyFrameState;

#define FRAME_STATE_SUSPENDED(S) ((S) == FRAME_SUSPENDED || (S) == FRAME_SUSPENDED_YIELD_FROM)
#define FRAME_STATE_CLOSED(S) ((S) >= FRAME_COMPLETED)
Copy link
Member

Choose a reason for hiding this comment

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

Does the state being FRAME_COMPLETED mean that it is closed?
Can't it be completed, but not closed?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no CLOSED state. There is COMPLETED and CLEARED. CLOSED is a new category of both. Maybe FINISHED is a better name for this.


enum _frameowner {
FRAME_OWNED_BY_THREAD = 0,
Expand Down
4 changes: 4 additions & 0 deletions Include/internal/pycore_opcode_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ extern "C" {
#define RESUME_OPARG_LOCATION_MASK 0x3
#define RESUME_OPARG_DEPTH1_MASK 0x4


#define RESUME_WITH_SUBITERATOR(V) \
(((V) & RESUME_OPARG_LOCATION_MASK) >= RESUME_AFTER_YIELD_FROM)

#ifdef __cplusplus
}
#endif
Expand Down
40 changes: 16 additions & 24 deletions Objects/genobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,10 @@
#include "pycore_genobject.h" // struct _Py_async_gen_state
#include "pycore_modsupport.h" // _PyArg_CheckPositional()
#include "pycore_object.h" // _PyObject_GC_UNTRACK()
#include "pycore_opcode_metadata.h" // _PyOpcode_Caches
#include "pycore_opcode_utils.h" // RESUME_AFTER_YIELD_FROM
#include "pycore_opcode_utils.h" // RESUME_WITH_SUBITERATOR
#include "pycore_pyerrors.h" // _PyErr_ClearExcState()
#include "pycore_pystate.h" // _PyThreadState_GET()

#include "opcode.h" // SEND
#include "frameobject.h" // _PyInterpreterFrame_GetLine
#include "pystats.h"

static PyObject *gen_close(PyGenObject *, PyObject *);
Expand Down Expand Up @@ -43,19 +40,12 @@ PyGen_GetCode(PyGenObject *gen) {
return res;
}

static inline int
exc_state_traverse(_PyErr_StackItem *exc_state, visitproc visit, void *arg)
{
Py_VISIT(exc_state->exc_value);
return 0;
}

static int
gen_traverse(PyGenObject *gen, visitproc visit, void *arg)
{
Py_VISIT(gen->gi_name);
Py_VISIT(gen->gi_qualname);
if (gen->gi_frame_state < FRAME_CLEARED) {
if (gen->gi_frame_state != FRAME_CLEARED) {
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)(gen->gi_iframe);
assert(frame->frame_obj == NULL ||
frame->frame_obj->f_frame->owner == FRAME_OWNED_BY_GENERATOR);
Expand All @@ -66,15 +56,16 @@ gen_traverse(PyGenObject *gen, visitproc visit, void *arg)
}
/* No need to visit cr_origin, because it's just tuples/str/int, so can't
participate in a reference cycle. */
return exc_state_traverse(&gen->gi_exc_state, visit, arg);
Py_VISIT(gen->gi_exc_state.exc_value);
return 0;
}

void
_PyGen_Finalize(PyObject *self)
{
PyGenObject *gen = (PyGenObject *)self;

if (gen->gi_frame_state >= FRAME_COMPLETED) {
if (FRAME_STATE_CLOSED(gen->gi_frame_state)) {
/* Generator isn't paused, so no need to close */
return;
}
Expand Down Expand Up @@ -147,7 +138,7 @@ gen_dealloc(PyGenObject *gen)
and GC_Del. */
Py_CLEAR(((PyAsyncGenObject*)gen)->ag_origin_or_finalizer);
}
if (gen->gi_frame_state < FRAME_CLEARED) {
if (gen->gi_frame_state != FRAME_CLEARED) {
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)gen->gi_iframe;
gen->gi_frame_state = FRAME_CLEARED;
frame->previous = NULL;
Expand All @@ -171,7 +162,6 @@ gen_send_ex2(PyGenObject *gen, PyObject *arg, PyObject **presult,
{
PyThreadState *tstate = _PyThreadState_GET();
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)gen->gi_iframe;
PyObject *result;

*presult = NULL;
if (gen->gi_frame_state == FRAME_CREATED && arg && arg != Py_None) {
Expand All @@ -198,7 +188,7 @@ gen_send_ex2(PyGenObject *gen, PyObject *arg, PyObject **presult,
PyErr_SetString(PyExc_ValueError, msg);
return PYGEN_ERROR;
}
if (gen->gi_frame_state >= FRAME_COMPLETED) {
if (FRAME_STATE_CLOSED(gen->gi_frame_state)) {
if (PyCoro_CheckExact(gen) && !closing) {
/* `gen` is an exhausted coroutine: raise an error,
except when called from gen_close(), which should
Expand All @@ -216,10 +206,12 @@ gen_send_ex2(PyGenObject *gen, PyObject *arg, PyObject **presult,
return PYGEN_ERROR;
}

assert(gen->gi_frame_state < FRAME_EXECUTING);
assert((gen->gi_frame_state == FRAME_CREATED) ||
FRAME_STATE_SUSPENDED(gen->gi_frame_state));

/* Push arg onto the frame's value stack */
result = arg ? arg : Py_None;
_PyFrame_StackPush(frame, Py_NewRef(result));
PyObject *arg_obj = arg ? arg : Py_None;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to require arg to be not NULL? This is a static function, so we know all the callers.
(Not necessarily in this PR, though)

_PyFrame_StackPush(frame, Py_NewRef(arg_obj));

_PyErr_StackItem *prev_exc_info = tstate->exc_info;
gen->gi_exc_state.previous_item = prev_exc_info;
Expand All @@ -232,7 +224,7 @@ gen_send_ex2(PyGenObject *gen, PyObject *arg, PyObject **presult,

gen->gi_frame_state = FRAME_EXECUTING;
EVAL_CALL_STAT_INC(EVAL_CALL_GENERATOR);
result = _PyEval_EvalFrame(tstate, frame, exc);
PyObject *result = _PyEval_EvalFrame(tstate, frame, exc);
assert(tstate->exc_info == prev_exc_info);
assert(gen->gi_exc_state.previous_item == NULL);
assert(gen->gi_frame_state != FRAME_EXECUTING);
Expand Down Expand Up @@ -349,7 +341,7 @@ _PyGen_yf(PyGenObject *gen)
if (gen->gi_frame_state == FRAME_SUSPENDED_YIELD_FROM) {
_PyInterpreterFrame *frame = (_PyInterpreterFrame *)gen->gi_iframe;
assert(is_resume(frame->instr_ptr));
assert((frame->instr_ptr->op.arg & RESUME_OPARG_LOCATION_MASK) >= RESUME_AFTER_YIELD_FROM);
assert(RESUME_WITH_SUBITERATOR(frame->instr_ptr->op.arg));
return Py_NewRef(_PyFrame_StackPeek(frame));
}
return NULL;
Expand All @@ -366,7 +358,7 @@ gen_close(PyGenObject *gen, PyObject *args)
gen->gi_frame_state = FRAME_COMPLETED;
Py_RETURN_NONE;
}
if (gen->gi_frame_state >= FRAME_COMPLETED) {
if (FRAME_STATE_CLOSED(gen->gi_frame_state)) {
Py_RETURN_NONE;
}
PyObject *yf = _PyGen_yf(gen);
Expand Down Expand Up @@ -2095,7 +2087,7 @@ async_gen_athrow_send(PyAsyncGenAThrow *o, PyObject *arg)
return NULL;
}

if (gen->gi_frame_state >= FRAME_COMPLETED) {
if (FRAME_STATE_CLOSED(gen->gi_frame_state)) {
o->agt_state = AWAITABLE_STATE_CLOSED;
PyErr_SetNone(PyExc_StopIteration);
return NULL;
Expand Down
4 changes: 2 additions & 2 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ dummy_func(
next_instr = this_instr;
}
else {
if ((oparg & RESUME_OPARG_LOCATION_MASK) < RESUME_AFTER_YIELD_FROM) {
if (!RESUME_WITH_SUBITERATOR(oparg)) {
CHECK_EVAL_BREAKER();
}
this_instr->op.code = RESUME_CHECK;
Expand Down Expand Up @@ -177,7 +177,7 @@ dummy_func(
next_instr = this_instr;
}
else {
if ((oparg & RESUME_OPARG_LOCATION_MASK) < RESUME_AFTER_YIELD_FROM) {
if (!RESUME_WITH_SUBITERATOR(oparg)) {
CHECK_EVAL_BREAKER();
}
_PyFrame_SetStackPointer(frame, stack_pointer);
Expand Down
4 changes: 2 additions & 2 deletions Python/generated_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.