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

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot 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

>