[cfe-commits] r164464 - /cfe/trunk/bindings/python/clang/cindex.py

[Tobias Grosser]

On 09/22/2012 09:23 PM, Gregory Szorc wrote:
> The new behavior is contrary to the logic documented in
> Config.set_compatibility_check:
>
> In case these bindings are used with an older version of libclang,
> parts that have been stable between releases may still work. Users of
> the python bindings can disable the compatibility check. This will
> cause the python bindings to load, even though they are written for a
> newer version of libclang. Failures now arise if unsupported or
> incompatible features are accessed. The user is required to test himself
 > if the features he is using are available and compatible between
> different libclang versions.

Hi Gregory,

sorry, I OK-ed that commit. From my point of view it was in line with 
the above comment.

> In summary, individual applications should trap the exception that was
> previously raised and make the decision that is right for them. i.e. the
> Python bindings will not make assumptions about whether callers should
> treat missing functionality as a fatal or acceptable. This patch breaks
> that contract and thus I think it should be reverted.

When I wrote the comment, I wanted to describe at which point a user 
will encounter an exception, in case an exception is actually thrown. I 
did not want to say, that missing functionality should always cause an 
exception. In fact, I had hoped that most incompatibilities could be 
handled within cindex.py, such that user code can avoid special casing 
for different libclang versions.

Especially in this case, returning an empty string seems to be a 
reasonable approach. Obviously, by returning an empty string, the user 
does not get the information that the brief comment is missing because 
of an old libclang version and not because the information is missing in 
the C code itself. However, I believe for most users this difference is 
not important. If the information is needed anyway, we can add methods 
to check if certain features are available. In this case, we could add 
conf.has_feature_completion_brief_comments().
Using feature check methods has the advantage, that users can query the 
availability of certain features without using the features. This will 
avoid the overhead, that would otherwise be caused by calling a method, 
throwing the FeatureDoesNotExist exception and catching this exception. 
Also, in some cases a user may want to know if a feature is available, 
even though he does not want to use the feature right now.

> This got me thinking, perhaps we should make the library instance a
> simple proxy type that converts KeyError into a MissingFunctionError or
> similar so callers can more easily trap missing libclang functions.

I think this is a good idea as a MissingFunctionError could also be used 
to provide better error messages to the user.

Gregory, does this explanation make sense or should we revert the patch 
and continue the discussion?

Cheers
Tobi