PR translation/90118 Missing space between words

Message ID CAKdteOaCsYMn87U8ZkKFHn-swGf46-R-KTyw4+BK14J1RB+TDQ@mail.gmail.com
State New
Headers show
Series
  • PR translation/90118 Missing space between words
Related show

Commit Message

Christophe Lyon April 18, 2019, 3:04 p.m.
Hi,

This patch adds the missing space before '%<' in
config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the
check-internal-format-escaping.py checker to warn about such cases.

OK?

Christophe

contrib/ChangeLog:

2019-04-18  Christophe Lyon  <christophe.lyon@linaro.org>

	PR translation/90118
	* check-internal-format-escaping.py: Check that %< is not next to
	a word.

gcc/ChangeLog:

2019-04-18  Christophe Lyon  <christophe.lyon@linaro.org>

	PR translation/90118
	* config/aarch64/aarch64.c (aarch64_override_options_internal):
	Add missing space before %<.

gcc/cp/ChangeLog:

2019-04-18  Christophe Lyon  <christophe.lyon@linaro.org>

	PR translation/90118
	* call.c (print_z_candidate): Add missing space before %<.

Comments

Richard Sandiford April 18, 2019, 4:25 p.m. | #1
Christophe Lyon <christophe.lyon@linaro.org> writes:
> Hi,

>

> This patch adds the missing space before '%<' in

> config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the

> check-internal-format-escaping.py checker to warn about such cases.

>

> OK?

>

> Christophe

>

> diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py

> index aac4f9e..9c62586 100755

> --- a/contrib/check-internal-format-escaping.py

> +++ b/contrib/check-internal-format-escaping.py

> @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

>                          print('%s: %s' % (origin, text))

>                      if re.search("[^%]'", p):

>                          print('%s: %s' % (origin, text))

> +                    # %< should not be preceded by a non-punctuation

> +                    # %character.

> +                    if re.search("[a-zA-Z0-9]%<", p):

> +                        print('%s: %s' % (origin, text))

>              j += 1

>  

>          origin = None

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> index 9be7548..b66071f 100644

> --- a/gcc/config/aarch64/aarch64.c

> +++ b/gcc/config/aarch64/aarch64.c

> @@ -11483,7 +11483,7 @@ aarch64_override_options_internal (struct gcc_options *opts)

>    if (aarch64_stack_protector_guard == SSP_GLOBAL

>        && opts->x_aarch64_stack_protector_guard_offset_str)

>      {

> -      error ("incompatible options %<-mstack-protector-guard=global%> and"

> +      error ("incompatible options %<-mstack-protector-guard=global%> and "

>  	     "%<-mstack-protector-guard-offset=%s%>",

>  	     aarch64_stack_protector_guard_offset_str);

>      }

> diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> index 9582345..8f3d019 100644

> --- a/gcc/cp/call.c

> +++ b/gcc/cp/call.c

> @@ -3614,16 +3614,16 @@ print_z_candidate (location_t loc, const char *msgstr,

>      {

>        cloc = loc;

>        if (candidate->num_convs == 3)

> -	inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,

> +	inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,

>  		candidate->convs[0]->type,

>  		candidate->convs[1]->type,

>  		candidate->convs[2]->type);

>        else if (candidate->num_convs == 2)

> -	inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,

> +	inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,

>  		candidate->convs[0]->type,

>  		candidate->convs[1]->type);

>        else

> -	inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,

> +	inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,

>  		candidate->convs[0]->type);

>      }

>    else if (TYPE_P (fn))


I don't think this is right, since msg already has a space where necessary:

  const char *msg = (msgstr == NULL
		     ? ""
		     : ACONCAT ((msgstr, " ", NULL)));

Adding something like "(^| )[^% ]*" to the start of the regexp might
avoid that, at the risk of letting through real problems.

The aarch64.c change is definitely OK though, thanks for the catch.

Richard
Christophe Lyon April 19, 2019, 9:01 a.m. | #2
On Thu, 18 Apr 2019 at 18:25, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>

> Christophe Lyon <christophe.lyon@linaro.org> writes:

> > Hi,

> >

> > This patch adds the missing space before '%<' in

> > config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the

> > check-internal-format-escaping.py checker to warn about such cases.

> >

> > OK?

> >

> > Christophe

> >

> > diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py

> > index aac4f9e..9c62586 100755

> > --- a/contrib/check-internal-format-escaping.py

> > +++ b/contrib/check-internal-format-escaping.py

> > @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

> >                          print('%s: %s' % (origin, text))

> >                      if re.search("[^%]'", p):

> >                          print('%s: %s' % (origin, text))

> > +                    # %< should not be preceded by a non-punctuation

> > +                    # %character.

> > +                    if re.search("[a-zA-Z0-9]%<", p):

> > +                        print('%s: %s' % (origin, text))

> >              j += 1

> >

> >          origin = None

> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> > index 9be7548..b66071f 100644

> > --- a/gcc/config/aarch64/aarch64.c

> > +++ b/gcc/config/aarch64/aarch64.c

> > @@ -11483,7 +11483,7 @@ aarch64_override_options_internal (struct gcc_options *opts)

> >    if (aarch64_stack_protector_guard == SSP_GLOBAL

> >        && opts->x_aarch64_stack_protector_guard_offset_str)

> >      {

> > -      error ("incompatible options %<-mstack-protector-guard=global%> and"

> > +      error ("incompatible options %<-mstack-protector-guard=global%> and "

> >            "%<-mstack-protector-guard-offset=%s%>",

> >            aarch64_stack_protector_guard_offset_str);

> >      }

> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> > index 9582345..8f3d019 100644

> > --- a/gcc/cp/call.c

> > +++ b/gcc/cp/call.c

> > @@ -3614,16 +3614,16 @@ print_z_candidate (location_t loc, const char *msgstr,

> >      {

> >        cloc = loc;

> >        if (candidate->num_convs == 3)

> > -     inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,

> > +     inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,

> >               candidate->convs[0]->type,

> >               candidate->convs[1]->type,

> >               candidate->convs[2]->type);

> >        else if (candidate->num_convs == 2)

> > -     inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,

> > +     inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,

> >               candidate->convs[0]->type,

> >               candidate->convs[1]->type);

> >        else

> > -     inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,

> > +     inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,

> >               candidate->convs[0]->type);

> >      }

> >    else if (TYPE_P (fn))

>

> I don't think this is right, since msg already has a space where necessary:

>

>   const char *msg = (msgstr == NULL

>                      ? ""

>                      : ACONCAT ((msgstr, " ", NULL)));

>

> Adding something like "(^| )[^% ]*" to the start of the regexp might

> avoid that, at the risk of letting through real problems.

>


Yes, that would miss the problem in aarch64.c.

> The aarch64.c change is definitely OK though, thanks for the catch.

>


I'll committ the aarch64.c and check-internal-format-escaping.py parts.

Thanks,

Christophe

> Richard
Richard Sandiford April 19, 2019, 9:23 a.m. | #3
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Thu, 18 Apr 2019 at 18:25, Richard Sandiford

> <richard.sandiford@arm.com> wrote:

>>

>> Christophe Lyon <christophe.lyon@linaro.org> writes:

>> > Hi,

>> >

>> > This patch adds the missing space before '%<' in

>> > config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the

>> > check-internal-format-escaping.py checker to warn about such cases.

>> >

>> > OK?

>> >

>> > Christophe

>> >

>> > diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py

>> > index aac4f9e..9c62586 100755

>> > --- a/contrib/check-internal-format-escaping.py

>> > +++ b/contrib/check-internal-format-escaping.py

>> > @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

>> >                          print('%s: %s' % (origin, text))

>> >                      if re.search("[^%]'", p):

>> >                          print('%s: %s' % (origin, text))

>> > +                    # %< should not be preceded by a non-punctuation

>> > +                    # %character.

>> > +                    if re.search("[a-zA-Z0-9]%<", p):

>> > +                        print('%s: %s' % (origin, text))

>> >              j += 1

>> >

>> >          origin = None

>> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

>> > index 9be7548..b66071f 100644

>> > --- a/gcc/config/aarch64/aarch64.c

>> > +++ b/gcc/config/aarch64/aarch64.c

>> > @@ -11483,7 +11483,7 @@ aarch64_override_options_internal (struct gcc_options *opts)

>> >    if (aarch64_stack_protector_guard == SSP_GLOBAL

>> >        && opts->x_aarch64_stack_protector_guard_offset_str)

>> >      {

>> > -      error ("incompatible options %<-mstack-protector-guard=global%> and"

>> > +      error ("incompatible options %<-mstack-protector-guard=global%> and "

>> >            "%<-mstack-protector-guard-offset=%s%>",

>> >            aarch64_stack_protector_guard_offset_str);

>> >      }

>> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c

>> > index 9582345..8f3d019 100644

>> > --- a/gcc/cp/call.c

>> > +++ b/gcc/cp/call.c

>> > @@ -3614,16 +3614,16 @@ print_z_candidate (location_t loc, const char *msgstr,

>> >      {

>> >        cloc = loc;

>> >        if (candidate->num_convs == 3)

>> > -     inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,

>> > +     inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,

>> >               candidate->convs[0]->type,

>> >               candidate->convs[1]->type,

>> >               candidate->convs[2]->type);

>> >        else if (candidate->num_convs == 2)

>> > -     inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,

>> > +     inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,

>> >               candidate->convs[0]->type,

>> >               candidate->convs[1]->type);

>> >        else

>> > -     inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,

>> > +     inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,

>> >               candidate->convs[0]->type);

>> >      }

>> >    else if (TYPE_P (fn))

>>

>> I don't think this is right, since msg already has a space where necessary:

>>

>>   const char *msg = (msgstr == NULL

>>                      ? ""

>>                      : ACONCAT ((msgstr, " ", NULL)));

>>

>> Adding something like "(^| )[^% ]*" to the start of the regexp might

>> avoid that, at the risk of letting through real problems.

>>

>

> Yes, that would miss the problem in aarch64.c.


Are you sure?  It works for me.

The idea is to treat any immediately-adjoining non-whitespace sequence as
punctuation rather than a word if it includes a % character.

>> The aarch64.c change is definitely OK though, thanks for the catch.

>>

>

> I'll committ the aarch64.c and check-internal-format-escaping.py parts.


I wasn't sure whether we wanted to avoid false positives, so that anyone
running the script doesn't need to repeat the same thought process.

Thanks,
Richard
Christophe Lyon April 19, 2019, 9:28 a.m. | #4
On Fri, 19 Apr 2019 at 11:23, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>

> Christophe Lyon <christophe.lyon@linaro.org> writes:

> > On Thu, 18 Apr 2019 at 18:25, Richard Sandiford

> > <richard.sandiford@arm.com> wrote:

> >>

> >> Christophe Lyon <christophe.lyon@linaro.org> writes:

> >> > Hi,

> >> >

> >> > This patch adds the missing space before '%<' in

> >> > config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the

> >> > check-internal-format-escaping.py checker to warn about such cases.

> >> >

> >> > OK?

> >> >

> >> > Christophe

> >> >

> >> > diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py

> >> > index aac4f9e..9c62586 100755

> >> > --- a/contrib/check-internal-format-escaping.py

> >> > +++ b/contrib/check-internal-format-escaping.py

> >> > @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

> >> >                          print('%s: %s' % (origin, text))

> >> >                      if re.search("[^%]'", p):

> >> >                          print('%s: %s' % (origin, text))

> >> > +                    # %< should not be preceded by a non-punctuation

> >> > +                    # %character.

> >> > +                    if re.search("[a-zA-Z0-9]%<", p):

> >> > +                        print('%s: %s' % (origin, text))

> >> >              j += 1

> >> >

> >> >          origin = None

> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> >> > index 9be7548..b66071f 100644

> >> > --- a/gcc/config/aarch64/aarch64.c

> >> > +++ b/gcc/config/aarch64/aarch64.c

> >> > @@ -11483,7 +11483,7 @@ aarch64_override_options_internal (struct gcc_options *opts)

> >> >    if (aarch64_stack_protector_guard == SSP_GLOBAL

> >> >        && opts->x_aarch64_stack_protector_guard_offset_str)

> >> >      {

> >> > -      error ("incompatible options %<-mstack-protector-guard=global%> and"

> >> > +      error ("incompatible options %<-mstack-protector-guard=global%> and "

> >> >            "%<-mstack-protector-guard-offset=%s%>",

> >> >            aarch64_stack_protector_guard_offset_str);

> >> >      }

> >> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> >> > index 9582345..8f3d019 100644

> >> > --- a/gcc/cp/call.c

> >> > +++ b/gcc/cp/call.c

> >> > @@ -3614,16 +3614,16 @@ print_z_candidate (location_t loc, const char *msgstr,

> >> >      {

> >> >        cloc = loc;

> >> >        if (candidate->num_convs == 3)

> >> > -     inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,

> >> > +     inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,

> >> >               candidate->convs[0]->type,

> >> >               candidate->convs[1]->type,

> >> >               candidate->convs[2]->type);

> >> >        else if (candidate->num_convs == 2)

> >> > -     inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,

> >> > +     inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,

> >> >               candidate->convs[0]->type,

> >> >               candidate->convs[1]->type);

> >> >        else

> >> > -     inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,

> >> > +     inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,

> >> >               candidate->convs[0]->type);

> >> >      }

> >> >    else if (TYPE_P (fn))

> >>

> >> I don't think this is right, since msg already has a space where necessary:

> >>

> >>   const char *msg = (msgstr == NULL

> >>                      ? ""

> >>                      : ACONCAT ((msgstr, " ", NULL)));

> >>

> >> Adding something like "(^| )[^% ]*" to the start of the regexp might

> >> avoid that, at the risk of letting through real problems.

> >>

> >

> > Yes, that would miss the problem in aarch64.c.

>

> Are you sure?  It works for me.

>

It didn't work for me, with that change the line didn't report any error.


> The idea is to treat any immediately-adjoining non-whitespace sequence as

> punctuation rather than a word if it includes a % character.

>

> >> The aarch64.c change is definitely OK though, thanks for the catch.

> >>

> >

> > I'll committ the aarch64.c and check-internal-format-escaping.py parts.

>

> I wasn't sure whether we wanted to avoid false positives, so that anyone

> running the script doesn't need to repeat the same thought process.

>

> Thanks,

> Richard
Jakub Jelinek April 19, 2019, 9:38 a.m. | #5
On Fri, Apr 19, 2019 at 11:28:41AM +0200, Christophe Lyon wrote:
> > >> > --- a/contrib/check-internal-format-escaping.py

> > >> > +++ b/contrib/check-internal-format-escaping.py

> > >> > @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

> > >> >                          print('%s: %s' % (origin, text))

> > >> >                      if re.search("[^%]'", p):

> > >> >                          print('%s: %s' % (origin, text))

> > >> > +                    # %< should not be preceded by a non-punctuation

> > >> > +                    # %character.

> > >> > +                    if re.search("[a-zA-Z0-9]%<", p):

> > >> > +                        print('%s: %s' % (origin, text))

> > >> >              j += 1


I'm not convinced we want this in check-internal-format-escaping.py
exactly because it is too fragile.
Instead, we want what David Malcolm has proposed, for GCC 10 some
-Wformat-something (non-default) style warning or even a warning for
all string literals when there is
"str "
" str2"
or
"str"
"str2"
Basically require if there is a line break between two concatenated string
literals that there is a single space, either at the end of one chunk or
at the beginning of the other one.

	Jakub
Richard Sandiford April 19, 2019, 9:57 a.m. | #6
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Fri, 19 Apr 2019 at 11:23, Richard Sandiford

> <richard.sandiford@arm.com> wrote:

>>

>> Christophe Lyon <christophe.lyon@linaro.org> writes:

>> > On Thu, 18 Apr 2019 at 18:25, Richard Sandiford

>> > <richard.sandiford@arm.com> wrote:

>> >>

>> >> Christophe Lyon <christophe.lyon@linaro.org> writes:

>> >> > Hi,

>> >> >

>> >> > This patch adds the missing space before '%<' in

>> >> > config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the

>> >> > check-internal-format-escaping.py checker to warn about such cases.

>> >> >

>> >> > OK?

>> >> >

>> >> > Christophe

>> >> >

>> >> > diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py

>> >> > index aac4f9e..9c62586 100755

>> >> > --- a/contrib/check-internal-format-escaping.py

>> >> > +++ b/contrib/check-internal-format-escaping.py

>> >> > @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

>> >> >                          print('%s: %s' % (origin, text))

>> >> >                      if re.search("[^%]'", p):

>> >> >                          print('%s: %s' % (origin, text))

>> >> > +                    # %< should not be preceded by a non-punctuation

>> >> > +                    # %character.

>> >> > +                    if re.search("[a-zA-Z0-9]%<", p):

>> >> > +                        print('%s: %s' % (origin, text))

>> >> >              j += 1

>> >> >

>> >> >          origin = None

>> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

>> >> > index 9be7548..b66071f 100644

>> >> > --- a/gcc/config/aarch64/aarch64.c

>> >> > +++ b/gcc/config/aarch64/aarch64.c

>> >> > @@ -11483,7 +11483,7 @@ aarch64_override_options_internal (struct gcc_options *opts)

>> >> >    if (aarch64_stack_protector_guard == SSP_GLOBAL

>> >> >        && opts->x_aarch64_stack_protector_guard_offset_str)

>> >> >      {

>> >> > -      error ("incompatible options %<-mstack-protector-guard=global%> and"

>> >> > +      error ("incompatible options %<-mstack-protector-guard=global%> and "

>> >> >            "%<-mstack-protector-guard-offset=%s%>",

>> >> >            aarch64_stack_protector_guard_offset_str);

>> >> >      }

>> >> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c

>> >> > index 9582345..8f3d019 100644

>> >> > --- a/gcc/cp/call.c

>> >> > +++ b/gcc/cp/call.c

>> >> > @@ -3614,16 +3614,16 @@ print_z_candidate (location_t loc, const char *msgstr,

>> >> >      {

>> >> >        cloc = loc;

>> >> >        if (candidate->num_convs == 3)

>> >> > -     inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,

>> >> > +     inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,

>> >> >               candidate->convs[0]->type,

>> >> >               candidate->convs[1]->type,

>> >> >               candidate->convs[2]->type);

>> >> >        else if (candidate->num_convs == 2)

>> >> > -     inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,

>> >> > +     inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,

>> >> >               candidate->convs[0]->type,

>> >> >               candidate->convs[1]->type);

>> >> >        else

>> >> > -     inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,

>> >> > +     inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,

>> >> >               candidate->convs[0]->type);

>> >> >      }

>> >> >    else if (TYPE_P (fn))

>> >>

>> >> I don't think this is right, since msg already has a space where necessary:

>> >>

>> >>   const char *msg = (msgstr == NULL

>> >>                      ? ""

>> >>                      : ACONCAT ((msgstr, " ", NULL)));

>> >>

>> >> Adding something like "(^| )[^% ]*" to the start of the regexp might

>> >> avoid that, at the risk of letting through real problems.

>> >>

>> >

>> > Yes, that would miss the problem in aarch64.c.

>>

>> Are you sure?  It works for me.

>>

> It didn't work for me, with that change the line didn't report any error.


Hmm, OK.  With:

                    if re.search("(^| )[^% ]*[a-zA-Z0-9]%<", p):
                        print('%s: %s' % (origin, text))

and a tree without your aarch64.c fix, I get:

#: config/aarch64/aarch64.c:11486: incompatible options %<-mstack-protector-guard=global%> and%<-mstack-"

but no warning about the cp/call.c stuff.

Thanks,
Richard
Christophe Lyon April 19, 2019, 12:33 p.m. | #7
On Fri, 19 Apr 2019 at 11:57, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>

> Christophe Lyon <christophe.lyon@linaro.org> writes:

> > On Fri, 19 Apr 2019 at 11:23, Richard Sandiford

> > <richard.sandiford@arm.com> wrote:

> >>

> >> Christophe Lyon <christophe.lyon@linaro.org> writes:

> >> > On Thu, 18 Apr 2019 at 18:25, Richard Sandiford

> >> > <richard.sandiford@arm.com> wrote:

> >> >>

> >> >> Christophe Lyon <christophe.lyon@linaro.org> writes:

> >> >> > Hi,

> >> >> >

> >> >> > This patch adds the missing space before '%<' in

> >> >> > config/aarch64/aarch64.c and gcc/cp/call.c. It also updates the

> >> >> > check-internal-format-escaping.py checker to warn about such cases.

> >> >> >

> >> >> > OK?

> >> >> >

> >> >> > Christophe

> >> >> >

> >> >> > diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py

> >> >> > index aac4f9e..9c62586 100755

> >> >> > --- a/contrib/check-internal-format-escaping.py

> >> >> > +++ b/contrib/check-internal-format-escaping.py

> >> >> > @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

> >> >> >                          print('%s: %s' % (origin, text))

> >> >> >                      if re.search("[^%]'", p):

> >> >> >                          print('%s: %s' % (origin, text))

> >> >> > +                    # %< should not be preceded by a non-punctuation

> >> >> > +                    # %character.

> >> >> > +                    if re.search("[a-zA-Z0-9]%<", p):

> >> >> > +                        print('%s: %s' % (origin, text))

> >> >> >              j += 1

> >> >> >

> >> >> >          origin = None

> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

> >> >> > index 9be7548..b66071f 100644

> >> >> > --- a/gcc/config/aarch64/aarch64.c

> >> >> > +++ b/gcc/config/aarch64/aarch64.c

> >> >> > @@ -11483,7 +11483,7 @@ aarch64_override_options_internal (struct gcc_options *opts)

> >> >> >    if (aarch64_stack_protector_guard == SSP_GLOBAL

> >> >> >        && opts->x_aarch64_stack_protector_guard_offset_str)

> >> >> >      {

> >> >> > -      error ("incompatible options %<-mstack-protector-guard=global%> and"

> >> >> > +      error ("incompatible options %<-mstack-protector-guard=global%> and "

> >> >> >            "%<-mstack-protector-guard-offset=%s%>",

> >> >> >            aarch64_stack_protector_guard_offset_str);

> >> >> >      }

> >> >> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c

> >> >> > index 9582345..8f3d019 100644

> >> >> > --- a/gcc/cp/call.c

> >> >> > +++ b/gcc/cp/call.c

> >> >> > @@ -3614,16 +3614,16 @@ print_z_candidate (location_t loc, const char *msgstr,

> >> >> >      {

> >> >> >        cloc = loc;

> >> >> >        if (candidate->num_convs == 3)

> >> >> > -     inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,

> >> >> > +     inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,

> >> >> >               candidate->convs[0]->type,

> >> >> >               candidate->convs[1]->type,

> >> >> >               candidate->convs[2]->type);

> >> >> >        else if (candidate->num_convs == 2)

> >> >> > -     inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,

> >> >> > +     inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,

> >> >> >               candidate->convs[0]->type,

> >> >> >               candidate->convs[1]->type);

> >> >> >        else

> >> >> > -     inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,

> >> >> > +     inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,

> >> >> >               candidate->convs[0]->type);

> >> >> >      }

> >> >> >    else if (TYPE_P (fn))

> >> >>

> >> >> I don't think this is right, since msg already has a space where necessary:

> >> >>

> >> >>   const char *msg = (msgstr == NULL

> >> >>                      ? ""

> >> >>                      : ACONCAT ((msgstr, " ", NULL)));

> >> >>

> >> >> Adding something like "(^| )[^% ]*" to the start of the regexp might

> >> >> avoid that, at the risk of letting through real problems.

> >> >>

> >> >

> >> > Yes, that would miss the problem in aarch64.c.

> >>

> >> Are you sure?  It works for me.

> >>

> > It didn't work for me, with that change the line didn't report any error.

>

> Hmm, OK.  With:

>

>                     if re.search("(^| )[^% ]*[a-zA-Z0-9]%<", p):

>                         print('%s: %s' % (origin, text))

>

> and a tree without your aarch64.c fix, I get:

>

> #: config/aarch64/aarch64.c:11486: incompatible options %<-mstack-protector-guard=global%> and%<-mstack-"

>

> but no warning about the cp/call.c stuff.

>


This works for me too. I don't know what I got wrong in my previous check.

So... what should I do? Apply you patch, or revert mine according to
Jakub's comments?
Improving the checker now would help fixing these annoying things
without waiting for gcc-10

Christophe

> Thanks,

> Richard
Martin Sebor April 19, 2019, 5:54 p.m. | #8
On 4/19/19 3:38 AM, Jakub Jelinek wrote:
> On Fri, Apr 19, 2019 at 11:28:41AM +0200, Christophe Lyon wrote:

>>>>>> --- a/contrib/check-internal-format-escaping.py

>>>>>> +++ b/contrib/check-internal-format-escaping.py

>>>>>> @@ -58,6 +58,10 @@ for i, l in enumerate(lines):

>>>>>>                           print('%s: %s' % (origin, text))

>>>>>>                       if re.search("[^%]'", p):

>>>>>>                           print('%s: %s' % (origin, text))

>>>>>> +                    # %< should not be preceded by a non-punctuation

>>>>>> +                    # %character.

>>>>>> +                    if re.search("[a-zA-Z0-9]%<", p):

>>>>>> +                        print('%s: %s' % (origin, text))

>>>>>>               j += 1

> 

> I'm not convinced we want this in check-internal-format-escaping.py

> exactly because it is too fragile.


I have a patch that adds a -Wformat-diag warning that can detect
some of these kinds of problems.  For cases where the missing space
is intentional it relies on a pair of adjacent directives, as in
"%s%<%>foo" or "%s%s" with the "foo" being an argument.

> Instead, we want what David Malcolm has proposed, for GCC 10 some

> -Wformat-something (non-default) style warning or even a warning for

> all string literals when there is

> "str"

> " str2"

> or

> "str"

> "str2"


-Wformat-diag doesn't handle this because it runs as part of -Wformat

> Basically require if there is a line break between two concatenated string

> literals that there is a single space, either at the end of one chunk or

> at the beginning of the other one.


...so that would still be a useful feature.

Martin

Patch

diff --git a/contrib/check-internal-format-escaping.py b/contrib/check-internal-format-escaping.py
index aac4f9e..9c62586 100755
--- a/contrib/check-internal-format-escaping.py
+++ b/contrib/check-internal-format-escaping.py
@@ -58,6 +58,10 @@  for i, l in enumerate(lines):
                         print('%s: %s' % (origin, text))
                     if re.search("[^%]'", p):
                         print('%s: %s' % (origin, text))
+                    # %< should not be preceded by a non-punctuation
+                    # %character.
+                    if re.search("[a-zA-Z0-9]%<", p):
+                        print('%s: %s' % (origin, text))
             j += 1
 
         origin = None
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 9be7548..b66071f 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -11483,7 +11483,7 @@  aarch64_override_options_internal (struct gcc_options *opts)
   if (aarch64_stack_protector_guard == SSP_GLOBAL
       && opts->x_aarch64_stack_protector_guard_offset_str)
     {
-      error ("incompatible options %<-mstack-protector-guard=global%> and"
+      error ("incompatible options %<-mstack-protector-guard=global%> and "
 	     "%<-mstack-protector-guard-offset=%s%>",
 	     aarch64_stack_protector_guard_offset_str);
     }
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 9582345..8f3d019 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -3614,16 +3614,16 @@  print_z_candidate (location_t loc, const char *msgstr,
     {
       cloc = loc;
       if (candidate->num_convs == 3)
-	inform (cloc, "%s%<%D(%T, %T, %T)%> <built-in>", msg, fn,
+	inform (cloc, "%s %<%D(%T, %T, %T)%> <built-in>", msg, fn,
 		candidate->convs[0]->type,
 		candidate->convs[1]->type,
 		candidate->convs[2]->type);
       else if (candidate->num_convs == 2)
-	inform (cloc, "%s%<%D(%T, %T)%> <built-in>", msg, fn,
+	inform (cloc, "%s %<%D(%T, %T)%> <built-in>", msg, fn,
 		candidate->convs[0]->type,
 		candidate->convs[1]->type);
       else
-	inform (cloc, "%s%<%D(%T)%> <built-in>", msg, fn,
+	inform (cloc, "%s %<%D(%T)%> <built-in>", msg, fn,
 		candidate->convs[0]->type);
     }
   else if (TYPE_P (fn))