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

[Tobias Grosser]

On 09/05/2012 11:24 PM, Gregory Szorc wrote:
> 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.

Yes, that is what I meant. There is an agreement to add new behavior by 
adding new functions, not by changing the semantic of existing ones. 
This means the existence of a function is equivalent to libclang 
supporting that feature. Token API 2 will come with new functions.

>>>> 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.

Makes sense. I attached an updated patch. It should work and may be OK 
to commit. I started to think if it would be better to make each entry 
an object of a FuncDescription class. This would remove this
ugly use of item[0], ..., item[4]. I will give this a rest for now. Feel 
free to commit these changes in case you like them.

Tobi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Report-function-when-something-is-failing.patch
Type: text/x-patch
Size: 13315 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120906/ac72cf12/attachment.bin>