Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)
On 10/03/17 23:36, David Malcolm wrote:
On Fri, 2017-03-10 at 09:24 +, 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
Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)
On Fri, 2017-03-10 at 09:24 +, 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.
Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)
On 10/03/17 09:30, Jakub Jelinek wrote:
On Fri, Mar 10, 2017 at 09:24:18AM +, 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.
We have tons of messages like:
invalid --param name %qs; did you mean %qs?
Thanks, then using a semicolon here is the right thing to do.
Kyrill
Jakub
Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)
On Fri, Mar 10, 2017 at 09:24:18AM +, 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.
We have tons of messages like:
invalid --param name %qs; did you mean %qs?
Jakub
Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)
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
Re: [PATCH 3/7] Remove trailing period from various diagnostic messages (PR translation/79923)
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).
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.
Perhaps commit everything except arm-builtins.c separately and deal with
this part with the ARM folks?
Jakub
