Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

On 10/03/17 23:36, David Malcolm wrote:
> On Fri, 2017-03-10 at 09:24 +0000, Kyrill Tkachov wrote:
>> On 10/03/17 06:24, Jakub Jelinek wrote:
>>> On Thu, Mar 09, 2017 at 12:45:25PM -0500, David Malcolm wrote:
>>>> gcc/ChangeLog:
>>>> 	PR translation/79923
>>>> 	* auto-profile.c (get_combined_location): Convert leading
>>>> 	character of diagnostics to lower case and remove trailing
>>>> period.
>>>> 	(read_profile): Likewise for various diagnostics.
>>>> 	* config/arm/arm-builtins.c (arm_expand_builtin): Remove
>>>> trailing
>>>> 	period from various diagnostics.
>>>> 	* config/arm/arm.c (arm_option_override): Likewise.
>>>> 	* config/msp430/msp430.c (msp430_expand_delay_cycles):
>>>> Likewise.
>>>> 	(msp430_expand_delay_cycles): Likewise.
>>> Mostly ok, but for
>>>
>>>> --- a/gcc/config/arm/arm-builtins.c
>>>> +++ b/gcc/config/arm/arm-builtins.c
>>>> @@ -2990,60 +2990,60 @@ arm_expand_builtin (tree exp,
>>>>    	      && (imm < 0 || imm > 32))
>>>>    	    {
>>>>    	      if (fcode == ARM_BUILTIN_WRORHI)
>>>> -		error ("the range of count should be in 0 to 32.
>>>>    please check the intrinsic _mm_rori_pi16 in code.");
>>>> +		error ("the range of count should be in 0 to 32.
>>>>    please check the intrinsic _mm_rori_pi16 in code");
>>> I wonder if this shouldn't use a semicolon space in the middle
>>> instead of dot space space (many times in the same file).
>> Is there a convention in GCC to use semicolons?
>> I'm okay with changing it to a semicolon (it's slightly better IMO)
>> as long as it's consistent
>> with the style GCC uses.
>>
>>> Also, for the benefit of translators, this might be better done as
>>> 		error ("the range of count should be in 0 to 32; please
>>> check the intrinsic %s in code",
>>> 		       "_mm_rori_pi16");
>>> so that there are fewer of these messages.
>>> Adding some ARM folks on this.
>> These iWMMXt builtins haven't been touched in ages and could do with
>> some TLC in general.
>> For example, I'm not a fan of having all these "please check the
>> intrinsic ..." messages.
>> If we've got a reference to the tree we're expanding, isn't there
>> some kind of error function
>> that will point to the location in the source that's causing the
>> error? I'd rather use that than
>> hardcoding the intrinsic names. This would also allow us to collapse
>> all these
>> if (fcode == <...>)
>>     error (...);
>> else if (fcode == <...>)
>>     error (...);
>> else if ...
>>
>> constructs.
>>
>> While we're at it:
>>
>>    	      if (fcode == ARM_BUILTIN_WSRLHI)
>> -		error ("the count should be no less than 0.  please
>> check the intrinsic _mm_srli_pi16 in code.");
>> +		error ("the count should be no less than 0.  please
>> check the intrinsic _mm_srli_pi16 in code");
>>
>>
>> Let's use "the count should be a non-negative integer" to be
>> consistent with the error reporting
>> for UInteger error messages.
>>
>>> Perhaps commit everything except arm-builtins.c separately and deal
>>> with
>>> this part with the ARM folks?
>> I agree. David, if you want to clean up the error reporting in these
>> intrinsics as a separate patch I'd be grateful.
>> Otherwise, could you please open a bugzilla ticket so we can track
>> this?
> I started looking at this, but realized I don't have the arm ISA
> expertise to touch this code (sorry), so I filed
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79995
> instead.

No worries, thanks for looking at this in the first place.

Kyrill

>