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

[Manuel Klimek]

On Mon, May 14, 2012 at 5:46 AM, Gregory Szorc <gregory.szorc at gmail.com>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.


I'd prefer that - to me defensive programming means to:
1. never crash for invalid user input
2. crash hard if there's an actual bug in the library

If the cursor should never be generated by a user (-> the contract of the
library is to not break that), I think we should use _tu and have the
AttributeError thrown if it's not there. Or I'm also fine with putting in
an assert that checks that the attribute is there and delivers a nicer
error in case it's missing.


>  +        # 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).


The question is: don't we just want to assert that all of those have a TU
(or are a TU)?


>
>
>> 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!
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120514/8dfcdc21/attachment.html>