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

[Manuel Klimek]

What's ERROR_OK needed for? It looks like it's not currently used; it
actually looks like it's impossible to ever raise an exception with
it...

+    def __init__(self, ptr=None, filename=None, index=None):
+        """Create a TranslationUnit instance.
+
+        Instances can be created in the following ways:
+
+          * By passing a pointer to a c_object_p instance via ptr. This is
+            an internal mechanism and should not be used outside of this
+            module.

This interface seems strange - why have the mixture of 2 constructors in one?

Cheers,
/Manuel

On Mon, Mar 5, 2012 at 12:58 AM, Gregory Szorc <gregory.szorc at gmail.com> 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.
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>