[Cython] weird code in argument unpacking (memoryview related?)

[Stefan Behnel]

mark florisson, 30.03.2012 14:25:
> On 30 March 2012 12:19, Stefan Behnel wrote:
>> this code in Nodes.py around line 3950, at the end of the DefNodeWrapper's
>> method generate_keyword_unpacking_code(), was added as part of the memory
>> view changes, back in July last year:
>>
>> """
>>        # convert arg values to their final type and assign them
>>        for i, arg in enumerate(all_args):
>>            if arg.default and not arg.type.is_pyobject:
>>                code.putln("if (values[%d]) {" % i)
>>            if arg.default and not arg.type.is_pyobject:
>>                code.putln('} else {')
>>                code.putln(
>>                    "%s = %s;" % (
>>                        arg.entry.cname,
>>                        arg.calculate_default_value_code(code)))
>>                if arg.type.is_memoryviewslice:
>>                    code.put_incref_memoryviewslice(arg.entry.cname,
>>                                                    have_gil=True)
>>                code.putln('}')
>> """
>>
>> By being overly complicated, it hides rather well what it's actually meant
>> to achieve. It doesn't do at all what its comment says and also refers to
>> the default value of the argument, which is already assigned way before
>> this place in the code. Therefore, I'd be surprised if it generated
>> anything but dead C code, because the case that "values[i]" is NULL should
>> never happen.
>>
>> Mark, could you comment on this? Is this just a left-over from a broken
>> merge or something?
> 
> Hm, IIRC I was fixing acquisition count errors for memoryview slices
> (of which there were many) for arguments with default values, I was
> cleaning up after the initial memoryview implementation.
> 
> All I did was add another case for memoryview slices in old code (I
> only wrote the last 3 lines, you wrote the other ones in 2008).

Well, the entire block appeared in your commit (and it looked suspiciously
like my own code in retrospect), so I assumed that you copied it from
somewhere or that it accidentally slipped in while handling a merge conflict.


> Very
> similar but more recent code (probably copied and pasted) can be found
> around line ~3730 in generate_tuple_and_keyword_parsing_code (instead
> of generate_keyword_unpacking_code).

Right - that one makes sense. The "values[i] == NULL" case is actually used
for non-Python default values. I'll add a comment.

The first method calls the latter, so the correct code is being executed
after the block in question. That makes it look like the one above is just
a redundant quirk. The fact that it went undetected should hint at a
missing test or missing refnanny usage, though. A duplicated incref would
imply a reference leak, and the fact that it only affects default arguments
would make it very hard to detect. Might be visible by using memory views
as default arguments in closures, now that Vitja's fix correctly handles those.


> There are tests for default arguments and acquisition count errors
> should fail immediately and hard, so I trust your judgement to remove
> any or all of this.

Given that the correct code is in the right place, I've deleted the copy. I
did it in a branch because I was rewriting a part of that code anyway, so
any attempt to fix it directly in the master would just unnecessarily lead
to collisions. The whole code is here, commit #6 being the cleanup:

https://github.com/cython/cython/pull/107

Stefan