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

[Gregory Szorc]

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.

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