[cfe-dev] CompilationDatabase support added to libclang and python binding
gregory.szorc at gmail.com
Fri Jun 15 09:53:50 PDT 2012
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.
* I like CompilateDatabase.fromDirectory, not createFromDirectory.
* Don't import * in the test. Instead, import specific symbols.
More information about the cfe-dev