[cfe-commits] [PATCH 4/4] [clang.py] TranslationUnit API improvements

[Manuel Klimek]

I like the factory methods much better! Thanks :)

ERROR_OK is still in there though?

+        if isinstance(unsaved_files, dict):
+            for k, v in unsaved_files.iteritems():
+                unsaved_normalized.append((k, v))

unsaved_normalized = unsaved_files.items()

While I see how this is convenient, why do we not require a list and
let people outside call .items() if they have a dict? I kind of
dislike all this type specific code, but that's more a gut feeling
than being able to point my finger at problems.

Also, what happens if we pass parameters of incorrect type to
TranslationUnit_parse? All that type checking in python code seems
somewhat strange...

Cheers,
/Manuel

On Mon, Apr 16, 2012 at 1:30 AM, Gregory Szorc <gregory.szorc at gmail.com> wrote:
> Here is a revamped patch. I think we'll all be much happier with it.
>
> Strangely, the unit test for TranslationUnit.save() on an invalid TU
> is now failing because clang_saveTranslationUnit() is no longer
> returning a non-0 error. I'm not sure if it is an error in the Python
> bindings, the test, or a change in behavior in libclang (possibly
> r153560 or r152192). Whatever it is should be resolved before this
> lands.
>
> On Sun, Apr 15, 2012 at 1:27 PM, Gregory Szorc <gregory.szorc at gmail.com> wrote:
>> On Fri, Apr 13, 2012 at 3:38 AM, Manuel Klimek <klimek at google.com> wrote:
>>> What's ERROR_OK needed for? It looks like it's not currently used; it
>>> actually looks like it's impossible to ever raise an exception with
>>> it...
>>
>> That's true. I added it for parity with the C API. It can safely be removed.
>>
>>> +    def __init__(self, ptr=None, filename=None, index=None):
>>> +        """Create a TranslationUnit instance.
>>> +
>>> +        Instances can be created in the following ways:
>>> +
>>> +          * By passing a pointer to a c_object_p instance via ptr. This is
>>> +            an internal mechanism and should not be used outside of this
>>> +            module.
>>>
>>> This interface seems strange - why have the mixture of 2 constructors in one?
>>
>> Why not?
>>
>> Unfortunately, I can't find any reputable style guidelines to defend
>> either perspective. The closest I have is
>> http://stackoverflow.com/questions/682504/what-is-a-clean-pythonic-way-to-have-multiple-constructors-in-python.
>> And, that seems to indicate a mix of an "overloaded" __init__ with
>> @classmethod is preferred. But, that's just one SO question.
>>
>> Is this particular case, a TranslationUnit is ultimately instantiated
>> from a c_object_p "ptr." If we limited __init__ to a single
>> instantiation mode, we'd have to pass a c_object_p and since these are
>> internal to the module, __init__ wouldn't be an external API. In other
>> words, we'd be throwing __init__ away. Since Python programmers look
>> to __init__ first, I think this would be inconvenient. From an
>> external perspective, TranslationUnit still only has 1 instantiation
>> mode. If it had more, I'd definitely favor adding @classmethods to
>> cover each. I'm not against it today: I just see no reason for it.
>>
>> Anyway, as I typed this, I realized that we need an additional
>> constructor mode: from source file (e.g. Index.parse). Let me code up
>> a new patch and we'll see what you think.