[cfe-commits] [PATCH 4/4] [clang.py] TranslationUnit API improvements

[Gregory Szorc]

On 3/4/2012 4:58 PM, Gregory Szorc wrote:
> * Index.parse() and Index.read() now raise a TanslationUnitLoadException
>   instead of returning None if a TranslationUnit could not be
>   instantiated. This is backwards incompatible.
> * Ability to save TranslationUnits via TranslationUnit.save().
> * TranslationUnit now holds onto Index instance that created. This means
>   the Index can't be GC'd until the TranslationUnit is itself GC'd,
>   making memory management thoughtless.
> * Richer TranslationUnit.__init__ support. e.g. can now create a
>   TranslationUnit without explicitly creating an Index.
> * Implemented Index.read() through TranslationUnit.__init__ and
>   documented API as deprecated.
> * Don't use [] as a default argument value, as the initial value used is
>   reused for the duration of the program.
> ---
>  bindings/python/clang/cindex.py                    |  193 +++++++++++++++++---
>  .../python/tests/cindex/test_translation_unit.py   |   75 ++++++++-
>  bindings/python/tests/cindex/util.py               |    4 +-
>  3 files changed, 246 insertions(+), 26 deletions(-)
> 
> ----
> 
> I realize this patch may be a little big. I tried keeping it split
> locally, but management of all the patches was getting to be quite
> cumbersome. I gave up. I apologize for my laziness.
> 
> The most controversial part of this patch will be the
> backwards-incompatible change to Index.parse() and Index.read(), which
> now raise an exception instead of returning None. I feel the API is
> more pleasant because (and I have no empirical evidence to back this
> up) that the overwhelming majority of consumers assume a TU will be
> properly created when they try to create one. These consumers are
> currently penalized because [good programmers] will add the necessary
> "if tu is not None" check every time they try to obtain a TU. This
> patch does away with that penalty. Of course, the exception may still
> be handled explicitly if so chosen and this will result in roughly the
> same number of lines for error checking. But, I have a hunch most
> consumers won't want to deal with it and will have the exception
> bubble up.
> 
> I held off adding exception throwing to TranslationUnit.reparse(). I
> can certainly add that if we want things to be consistent everywhere.
> Or, I can revert the exception throwing altogether. Or, maybe I could
> make the new TranslationUnit.__init__ raise and have the old Index
> APIs swallow the exception and keep returning None. Choices.
> 
> I'd eventually like to roll Index.parse's main implementation into
> TranslationUnit.__init__, effectively deprecating Index.parse. I've
> already done this with Index.read() because it was trivial. I held off
> because I wanted to get blessing first. In the long term I'd like to
> move away from APIs on Index and towards the use of constructors on
> other objects. If Index had more direct APIs, I could be convinced
> otherwise. But, as it stands, it just feels that Index is unnecessary
> baggage in Python land. If we can make its existence invisible, I
> think that's a net win. This is why I've marked Index.read() as
> deprecated. I can certainly remove that comment if people don't agree
> with me.

Ping?