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

[Arnaud de Grandmaison]

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