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

[Kyrill Tkachov]

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?

Thanks,
Kyrill

        Jakub