[cfe-commits] [PATCH 1/4] [clang.py] Store reference to TranslationUnit in Cursor and Type

[Gregory Szorc]

Let's see if the mail systems don't reject this one...

On Sun, May 13, 2012 at 8:51 PM, Gregory Szorc <gregory.szorc at gmail.com> wrote:
> Fresh patch attached.
>
>
> On 5/13/12 8:46 PM, Gregory Szorc wrote:
>>
>> On 5/13/12 1:00 AM, Manuel Klimek wrote:
>>>
>>> +    @property
>>> +    def translation_unit(self):
>>> +        """Returns the TranslationUnit to which this Cursor belongs."""
>>> +        return getattr(self, '_tu', None)
>>>
>>> What's the reason for the default value? Do we expect that people
>>> create cursers via the lowlevel calls? Why would just return tu_ not
>>> work?
>>
>> Just defensive programming. If you return self._tu, that may raise an
>> AttributeError if _tu is not set. Theoretically, the API should ensure
>> that a TU is defined on all Cursor instances, so maybe an AttributeError
>> would be acceptable.
>>
>>> +        # Store a reference to the TU in the Python object so it
>>> won't get GC'd
>>> +        # before the Cursor.
>>> +        tu = None
>>> +        for arg in args:
>>> +            if isinstance(arg, TranslationUnit):
>>> +                tu = arg
>>> +                break
>>> +
>>> +            if hasattr(arg, 'translation_unit'):
>>> +                tu = arg.translation_unit
>>> +                break
>>> +
>>> +        assert tu is not None
>>> +
>>> +        res._tu = tu
>>>
>>> That seems - odd. I can't find docs what from_result is supposed to
>>> do, or what "args" are provided. But having to search through them for
>>> a TU seems wrong - shouldn't they all have a TU?
>>
>> from_result is the errcheck function for the ctypes functions that
>> return a Cursor. The 3rd argument to an errcheck function are the
>> original arguments passed into the called function.  For many of the
>> functions, the original argument is a Cursor. However,
>> clang_getTypeDeclaration takes a Type and clang_getTranslationUnitCursor
>> receives a TranslationUnit.
>>
>> It is true that all of the functions today receive a single argument, so
>> the iteration isn't required. However, that may change with the addition
>> of new APIs in the future (this is how I coded it in my nearly
>> feature-complete branch, so I'm guessing it is actually required from a
>> future patch).
>>>
>>>
>>> Also, regarding the test - would it be possible to create a test that
>>> drops the reference, triggers the gc and then makes sure the cursor is
>>> still valid (as that is what we really care about)?
>>>
>> Sure!
>>
>
>