Updated: [Patch, c* ,ObjC*] handle string objects in format checking.

[Nicola Pero]

Iain

I have been working on the "format" attributes for Objective-C methods today and the main missing attribute
for Objective-C is really __attribute__ ((format (NSString, 1, 2))).  That attribute would be so incredibly
useful (I have been wanting to have it since I started using Objective-C 10 years ago). ;-)

Your patch does much of it (thanks a lot for you work on that), but not all of it.  Are you planning 
to finish it so we can include it in 4.6.0 ? :-)

I know Joseph was asking what the new format is - well, all we need is:

 * a new format type (NSString), identical to 'printf', which the exception that:

   - it is only available in Objective-C / Objective-C++

   - it accepts the additional syntax %@, which should match an Objective-C object.  Arguments matching %@ 
     are good if (objc_is_object_ptr(argument)).

   - the format string is not a C string, but an Objective-C string.  If it is a constant Objective-C string 
     (ie, generated by the parser using objc_build_string_object()), we can peek in, extract the C string
     and the rest is identical to the other format types.  If it is not a constant Objective-C string, we 
     can only check that the format string is of the right type; you've already added an objc_string_ref_type_p()
     that does that (PS: we may want to polish that check, but it can be done as a bugfix)

Your patch does most of it, and with a bit of polishing and a few additions could do it all.  I know that
stage 1 is closing today, but this patch was in the making, and I'd say it would be worth finishing it for
inclusion (unless Mike or Joseph disagree of course). :-)

I would really be good to have __attribute__ ((format (NSString, 1, 2))) in GCC 4.6.0.

Thanks

[by the way, I can help with writing the code, but I don't think you need help with that ...] ;-)


-----Original Message-----
From: "IainS" <developer@sandoe-acoustics.co.uk>
Sent: Thursday, 28 October, 2010 21:43
To: "Joseph S. Myers" <joseph@codesourcery.com>
Cc: "GCC Patches" <gcc-patches@gcc.gnu.org>, "Mike Stump" <mrs@gcc.gnu.org>, "Mike Stump" <mikestump@comcast.net>
Subject: Re: Updated: [Patch, c* ,ObjC*] handle string objects in format checking.


On 28 Oct 2010, at 19:21, Joseph S. Myers wrote:

> On Thu, 28 Oct 2010, IainS wrote:
>
>> 	* doc/extend.tex (format): Document NSString extension.
>
> It's .texi not .tex.

Indeed, a typo, and I should also have put:

	PR target/44981

at the start of each segment.

>> +	  /* We expected a char but found an extended string type.  */
>> +	  if (is_objc_sref)
>> +	    error ("found a %<%s%> ref. but the format argument should be"
>> +		   " a string", format_name (gcc_objc_string_format_type));
>
> What is "ref."?  Is it a term of art in the relevant specification?   
> Or
> does the relevant specification say "reference"?  If "reference" is  
> the
> standard form, use that in the diagnostic.

Darwin consistently uses "CFStringRef" rather than CFString * (the  
CFString type itself is not available in end-user headers).
However, NSString is consistently referenced by NSString * (since  
NSString is a class).

Since the diagnostics have no cognizance of this, I was trying to make  
a solution that might seem more familiar to the recipient.

Since there's no standard as such, I've changed them all to  
'reference' since that's correct en.

>> +	    error ("found a %qD but the format argument should be a  
>> string",
>> +		   TYPE_NAME (ref));
>
> Will TYPE_NAME always be non-NULL here?  Or should you use %qT?
changed to qT.

>   In general you
> should check the formatting of this patch carefully; there seem to be
> several whitespace peculiarities.
I've re-scanned it and caught one more missing space.

>> +static const char *
>> +format_name (int format_num)
>
> Missing a comment describing the semantics of this function, its  
> argument
> and return value.
done

>> +{
>> +  if (format_num >= 0 && format_num < n_format_types)
>> +    return format_types[format_num].name;
>> +  else
>> +    return "invalid format type";
>
> No, returning an English fragment not marked up for translation is  
> not OK.
> If there is any valid or invalid input for which this case may be
> returned, please try to return a special value the caller can use to  
> pass
> an alternative full sentence to a diagnostic function; otherwise use
> gcc_unreachable here.
changed to gcc_unreachable (also in format_flags ()).

>> +static int
>> +format_flags (int format_num)
>
> Likewise needs comment.
done

>> +      /* Else we can't handle it and retire quietly.  */
>> +      return ;
>
> Stray space before ;.
done

>> @@ -1430,7 +1590,6 @@ check_format_arg (void *ctx, tree format_tree,
>>   free_alloc_pool (fwt_pool);
>> }
>>
>> -
>> /* Do the main part of checking a call to a format function.   
>> FORMAT_CHARS
>>    is the NUL-terminated format string (which at this point may  
>> contain
>>    internal NUL characters); FORMAT_LENGTH is its length (excluding  
>> the
>
> Bogus diff hunk changing whitespace in code not otherwise touched.
done

I also realized that the copyright years needed updating on several of  
the files and the amended attachment has that addition too.

re-tested on darwin with the test-case text amendments to reflect the  
use of 'reference'.

Iain




>
> -- 
> Joseph S. Myers
> joseph@codesourcery.com