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

[Gregory Szorc]

On Tue, May 15, 2012 at 12:45 PM, Manuel Klimek <klimek at google.com> wrote:
> On Tue, May 15, 2012 at 9:40 PM, Gregory Szorc <gregory.szorc at gmail.com> wrote:
>> On Tue, May 15, 2012 at 12:31 PM, Manuel Klimek <klimek at google.com> wrote:
>>> On Mon, May 14, 2012 at 10:15 AM, Manuel Klimek <klimek at google.com> wrote:
>>>> 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)?
>>>
>>> ^ this question is still open I think...
>>
>> I don't think it is. Both modified from_result functions now assert if
>> a TU could not be found.
>>
>> Or, are you asking for something more? In the future, every live
>> object will likely hold a reference to a TU. We aren't there quite
>> yet. But, this patch is a step in the right direction.
>
> Ah, ok, that was my question - why not every object has a TU and we
> can assert *for every object* that it has a TU. If that's a follow-up
> step, then LGTM.

Well, in the case of from_result (or any errcheck function), not every
argument to the original function may have a TU attached. e.g. integer
arguments.

Anyway...

Committing to https://llvm.org/svn/llvm-project/cfe/trunk ...
	M	bindings/python/clang/cindex.py
	M	bindings/python/tests/cindex/test_cursor.py
	M	bindings/python/tests/cindex/test_type.py
Committed r156846