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

[David Malcolm <dmalcolm at redhat dot com>]

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.