[cfe-dev] CompilationDatabase support added to libclang and python binding

[Gregory Szorc]

The Python looks good to me.

On Tue, Jun 19, 2012 at 1:19 AM, Arnaud de Grandmaison
<arnaud.allarddegrandmaison at parrot.com> wrote:
> Hi Manuel & Gregory,
>
> Here are the updated patches.
>
> The only significant change is in the python binding, where a
> CompilationCommand holds a reference to the associated
> CompilationCommands, preventing garbage collection. This is now also tested.
>
> Cheers,
> --
> Arnaud de Grandmaison
>
> On 06/18/2012 09:52 AM, Arnaud de Grandmaison wrote:
>> On 06/15/2012 06:53 PM, Gregory Szorc wrote:
>>> On 6/15/12 6:47 AM, Arnaud de Grandmaison wrote:
>>>> Hi Manuel & Gregory,
>>>>
>>>> If there are no more feedback, can someone commit those patches for me ?
>>>> I do not have commit access.
>>> Sorry about the delay.
>>>
>>> I'm not qualified to review the C++. But, the style changes look very
>>> good! I'm more than satisfied with its state.
>>>
>>> The Python is *extremely* close. The only major item left (and I didn't
>>> mention this in the initial feedback, sorry) is to store a reference to
>>> the associated CompilationDatabase in CompileCommands instances. Now,
>>> this may not be required (I haven't looked at the C++). If
>>> CompileCommands don't internally use a reference to their associated DB,
>>> we're fine. But if they do, then it is possible that a garbage
>>> collection could collect the DB instance and CompileCommands could blow
>>> up on next access. By stuffing a reference to the parent in the child,
>>> you prevent GC from prematurely collecting the parent. This manual
>>> reference tracking can be done in the from_result(). Just iterate over
>>> args until you find an instance of the appropriate type.
>>>
>>> Even if there is no reference now, it may not always work. You should
>>> add a test that forces a GC to ensure things don't blow up in the
>>> future. Search for "gc" in the test code and you'll find an example.
>>>
>>> Why does CompilationCommands.from_result raise a KeyError? KeyError
>>> would be appropriate if you were trying to access an attribute, etc. Its
>>> use on a function is IMO inappropriate. If you must raise, I think it
>>> should be a regular Exception. But, I think in this case the "Pythonic"
>>> thing to do is return None.
>>>
>>> Other nits:
>>>
>>> * I like CompilateDatabase.fromDirectory, not createFromDirectory.
>>> * Don't import * in the test. Instead, import specific symbols.
>>>
>>>
>> CompilationCommands and CompilationDatabase are completely independent
>> memory areas, but this may not be the case for CompilationCommand and
>> CompilationCommands. I have to check if we get a copy of the string.
>> Nevertheless, I will add the gc checks for CompilationCommand(s). Having
>> them to catch errors if implementation changes is good.
>>
>> For the KeyError, I wanted to differentiate between existing entries
>> (with no associated data) in the database, and non existing keys. Not
>> sure if this is really worth though. I am not a python expert, and I
>> will gladly make it more pythonic.
>>
>> I will update the patches.
>>
>
>
> --
> Arnaud de Grandmaison
> Senior CPU engineer
> Business Unit Digital Tuner
>
> Parrot S.A.
> 174, quai de Jemmapes
> 75010 Paris - France
> Phone: +33 1 48 03 84 59
>