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

[Gregory Szorc]

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.