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

[Arnaud de Grandmaison]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-cindex-cdb.patch
Type: text/x-patch
Size: 12938 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120619/dedd2e58/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-cindex-cdb-python.patch
Type: text/x-patch
Size: 9310 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120619/dedd2e58/attachment-0001.bin>