r196629 - ObjectiveC. Continuing implementation of objc_bridge_related

[jahanian]

Diagnostics improved in r196828. fixits are planned and will come in a later patch.

- Thanks, Fariborz

On Dec 9, 2013, at 9:24 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Dec 6, 2013, at 16:34 , Fariborz Jahanian <fjahanian at apple.com> wrote:
> 
>> +// objc_bridge_related attribute diagnostics.
>> +def err_objc_bridged_related_invalid_class : Error<
>> +  "could not find ObjectiveC class %0 to convert %1 to %2">;
>> +def err_objc_bridged_related_invalid_class_name : Error<
>> +  "%0 must be name of an ObjectiveC class to be able to convert %1 to %2">;
>> +def err_objc_bridged_related_class_method : Error<
>> +  "class method %0 for conversion of CF type %1 to an "
>> + "ObjectiveC object of type %2 not found">;
>> +def err_objc_bridged_related_instance_method : Error<
>> +  "instance method %0 for conversion of an ObjectiveC type %1 to a CF "
>> + "object  of type %2 not found">;
> 
> I don't think this is the right message to show to a user. The actual error is something like "you can't convert an X to a Y without using method -Z". The fact that method -Z isn't visible right now probably just means the user hasn't imported a header.
> 
> I understand the motivation to help framework authors get the annotations right, but I think we still have to phrase all our diagnostics for the application programmer. I would just unconditionally use the diagnostic below, but only use the fix-it if the method is visible.
> 
> 
>> +def err_objc_bridged_related_known_method : Error<
>> + "%0 must be explicitly converted to %1, use %2 method for this conversion">;
> 
> Semicolon instead of comma here? (And..."%select{%objcclass2|%objcinstance2}3", maybe? I think the "-" or "+" is important.)
> 
> 
>> +def err_objc_bridged_related_unknown_method : Error<
>> + "%0 must be explicitly converted to %1, define and then use "
>> + "%select{a singular class|an instance}3 method in %2 for this conversion">;
> 
> 
> I don't think this is the correct fix in most cases. Most of the time people are just trying to convert things that don't have an established conversion path, and they're not going to go add a category to the system frameworks to do it. I think I would just stick to a generic error here.
> 
> Also, "Objective-C", please, for all of these.
> 
> 
>> +  foo(newColor); // expected-error {{'CGColorRef' (aka 'struct CGColor *') must be explicitly converted to 'NSColor *', use 'colorWithCGColor:' method for this conversion}} \
>> +		 // expected-error {{implicit conversion of C pointer type 'CGColorRef' (aka 'struct CGColor *') to Objective-C pointer type 'NSColor *' requires a bridged cast}} \
>> +		 // expected-note {{use __bridge to convert directly (no change in ownership)}} \
>> +                 // expected-note {{use __bridge_transfer to transfer ownership of a +1 'CGColorRef' (aka 'struct CGColor *') into ARC}}
> 
> QoI: We really only need the first error, and that should have a fix-it that inserts the method call, and then from then on we compile as if the user had inserted it. (But since it's an error, we won't get past -fsyntax-only anyway, which is correct.)
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131209/bfce7960/attachment.html>