[cfe-commits] [PATCH] cindex.py - Make the compatibility check in cindex.py explicit

[Gregory Szorc]

On 9/5/12 2:05 PM, Tobias Grosser wrote:
> On 09/05/2012 10:55 PM, Gregory Szorc wrote:
>> On 9/5/12 1:29 PM, Tobias Grosser wrote:
>>> On 09/05/2012 09:07 PM, Gregory Szorc wrote:
>>>> I like it! Thank you for doing this.
>>>>
>>>> Specifically, I like how it defaults to strict and users must 
>>>> explicitly
>>>> choose to pull out a foot gun.
>>>>
>>>> There's probably potential for hooking up clang_getClangVersion() to
>>>> make the compatibility check even stricter. I'm think that the Python
>>>> bindings would contain a minimal libclang version. This way, if 
>>>> function
>>>> semantics ever change between libclang releases (I know the API is
>>>> supposed to be backwards compatible, but you never know), we can 
>>>> detect
>>>> that. Don't let this hold up committing this patch though.
>>>
>>> Thanks, committed in r163238.
>>>
>>> Regarding using clang_getClangVersion(). The comment says:
>>>
>>>  * \brief Return a version string, suitable for showing to a user, but
>>>  *        not intended to be parsed (the format is not guaranteed to be
>>>  *        stable).
>>>
>>> So parsing it to check the version sounds dangerous? We may need to
>>> establish another way to check for the libclang version? Maybe using a
>>> function, that just gives a version number?
>> Adding a function to get the version of libclang is a good idea. We
>> could go so far as to have libclang and the Python bindings store the
>> SVN revision they were built with. If there is a mismatch, it aborts.
>> This is probably too far for most users, especially since the Python
>> bindings aren't packaged by many distros today. I think a simple
>> major.minor comparison should be sufficient.
>>
>> That being said, I think I brought this up a while back (not sure if it
>> was on this list or IRC) and there was some push back. Apparently some
>> thought that because libclang is supposed to be backwards compatible
>> that a "get version" API was silly. My memory could be wrong here.
>
> My personal concern is how to get this right in presence of vendor 
> versions. The version scheme of Apple may conflict with the versions 
> of the opensource libclang. Users may want to target both of them.
Good point.
>
> Another way to go could be to detect the "version" of libclang in 
> terms of the available features. Basically, we detect which functions 
> are exposed and based on this we detect the features that we can 
> enable. My guess would be that each incompatibility probably came with 
> some new functions. Based on their presence/absence we could detect if 
> a feature is in sync.
 From a high level, I'd say that features should be exposed by a set of 
feature flags. If the semantics of a feature changes, that's a new 
unique flag. Translating that to C, we could have a bitfield or a 
function that takes a feature constant or string and returns whether it 
is present. Clients could say "do you support a token API." If the 
semantics of the token API ever changed, the client would say "do you 
support token API version 2." If that feature is supported, the client 
can enable support for it.

This is a little more robust than simple function presence because there 
is a contract around function behavior. You don't get that with just 
function presence (unless you change your function names when you change 
their behavior, which is probably a good idea anyway).

All that being said, I'm not sure we need this extra validation because 
libclang has a stable API and AFAIK function names are changed if 
behavior changes.
>
>>> Another idea, I was playing with can be seen in the attached patch. I
>>> did not finish thinking about it, so I don't yet submit it as an
>>> official patch, but you may want to have a look.
>> That's a great idea! User-friendly and actionable error messages are
>> always good.
>
> Great. The patch still needs some love . We need e.g. to decide if we 
> want the version info being part of a separate table or if it should 
> be merged with the method signature table. What do you think?
I think it should live in the method signature table. Related data 
should live together.