[Python-Dev] Preserving the blamelist

[Tim Peters]

[Tim]
>> Phillip, when eyeballing gen_dealloc(), I didn't understand two things:
>>
>> 1. Why doesn't
>>
>>         if (gen->gi_frame->f_stacktop!=NULL) {
>>
>>    check first to be sure that gen->gi_frame != Py_None?

[Phillip]
> Apparently, it's because I'm an idiot, and because nobody else realized
> this during the initial review of the patch.  :)

Then you were the bold adventurer, and the reviewers were the idiots ;-)

>> Is that impossible here for some reason?

> No, I goofed.  It's amazing that this doesn't dump core whenever a
> generator exits.  :(

Well, it's extremely likely that &((PyGenObject *)Py_None)->f_stacktop
is a legit part of the process address space, so that dereferencing is
non-problematic. We just read up nonsense then, test against it, and
the conditional

		gen->ob_type->tp_del(self);

is harmless as gen_del() returns at once (because it does check for
Py_None right away -- we never try to treat gi_frame->f_stacktop _as_
a frame pointer here, we just check it against NULL).  If we have to
have insane code, harmlessly insane is the best kind :--)

>> 2. It _looks_ like "gi_frame != NULL" is an (undocumented) invariant.
>>    Right?  If so,
>>
>>         Py_XDECREF(gen->gi_frame);
>>
>>    sends a confusing message (because of the "X", implying that NULL is OK).
>>    Regardless, it would be good to add comments to genobject.h explaining
>>    the possible values gi_frame can hold.  For example, what does it mean
>>    when gi_frame is Py_None?  Can it ever be NULL?

> I think what happened is that at one point I thought I was going to set
> gi_frame=NULL when there's no active frame, in order to speed up
> reclamation of the frame.  However, I think I then thought that it would
> break the operation of the generator's 'gi_frame' attribute, which is
> defined as T_OBJECT.

That shouldn't be a problem:  when PyMember_GetOne() fetches a
T_OBJECT that's NULL, it returns (a properly incref'ed) Py_None
instead.

> Or else I thought that the tp_visit was screwed up in that case.
>
> So, it looks to me like what this *should* do is simply allow gi_frame to
> be NULL instead of Py_None, which would get rid of all the silly casting,
> and retroactively make the XDECREF correct.  :)

That indeed sounds better to me, although you still don't get out of
writing gi_frame comments for genobject.h :-)

> Does gen_traverse() need to do anything special to visit a null
> pointer?  Should it only conditionally visit it?

The body of gen_traverse() is best written:

    Py_VISIT(gen->gi_frame);
    return 0;

Py_VISIT is defined in objimpl.h, and takes care of NULLs and
exceptional returns.

BTW, someone looking for an easy task might enjoy rewriting other
tp_traverse slots to use Py_VISIT.  We even have cases now (like
super_traverse) where modules define their own workalike
traverse-visit macros, which has become confusing since a standard
macro was defined for this purpose.