Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Martin Sebor

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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Christophe Lyon
On Fri, 19 Apr 2019 at 11:57, Richard Sandiford
 wrote:
>
> Christophe Lyon  writes:
> > On Fri, 19 Apr 2019 at 11:23, Richard Sandiford
> >  wrote:
> >>
> >> Christophe Lyon  writes:
> >> > On Thu, 18 Apr 2019 at 18:25, Richard Sandiford
> >> >  wrote:
> >> >>
> >> >> Christophe Lyon  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)%> ", msg, fn,
> >> >> > + inform (cloc, "%s %<%D(%T, %T, %T)%> ", 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)%> ", msg, fn,
> >> >> > + inform (cloc, "%s %<%D(%T, %T)%> ", msg, fn,
> >> >> >   candidate->convs[0]->type,
> >> >> >   candidate->convs[1]->type);
> >> >> >else
> >> >> > - inform (cloc, "%s%<%D(%T)%> ", msg, fn,
> >> >> > + inform (cloc, "%s %<%D(%T)%> ", 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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Richard Sandiford
Christophe Lyon  writes:
> On Fri, 19 Apr 2019 at 11:23, Richard Sandiford
>  wrote:
>>
>> Christophe Lyon  writes:
>> > On Thu, 18 Apr 2019 at 18:25, Richard Sandiford
>> >  wrote:
>> >>
>> >> Christophe Lyon  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)%> ", msg, fn,
>> >> > + inform (cloc, "%s %<%D(%T, %T, %T)%> ", 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)%> ", msg, fn,
>> >> > + inform (cloc, "%s %<%D(%T, %T)%> ", msg, fn,
>> >> >   candidate->convs[0]->type,
>> >> >   candidate->convs[1]->type);
>> >> >else
>> >> > - inform (cloc, "%s%<%D(%T)%> ", msg, fn,
>> >> > + inform (cloc, "%s %<%D(%T)%> ", 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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Jakub Jelinek
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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Christophe Lyon
On Fri, 19 Apr 2019 at 11:23, Richard Sandiford
 wrote:
>
> Christophe Lyon  writes:
> > On Thu, 18 Apr 2019 at 18:25, Richard Sandiford
> >  wrote:
> >>
> >> Christophe Lyon  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)%> ", msg, fn,
> >> > + inform (cloc, "%s %<%D(%T, %T, %T)%> ", 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)%> ", msg, fn,
> >> > + inform (cloc, "%s %<%D(%T, %T)%> ", msg, fn,
> >> >   candidate->convs[0]->type,
> >> >   candidate->convs[1]->type);
> >> >else
> >> > - inform (cloc, "%s%<%D(%T)%> ", msg, fn,
> >> > + inform (cloc, "%s %<%D(%T)%> ", 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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Richard Sandiford
Christophe Lyon  writes:
> On Thu, 18 Apr 2019 at 18:25, Richard Sandiford
>  wrote:
>>
>> Christophe Lyon  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)%> ", msg, fn,
>> > + inform (cloc, "%s %<%D(%T, %T, %T)%> ", 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)%> ", msg, fn,
>> > + inform (cloc, "%s %<%D(%T, %T)%> ", msg, fn,
>> >   candidate->convs[0]->type,
>> >   candidate->convs[1]->type);
>> >else
>> > - inform (cloc, "%s%<%D(%T)%> ", msg, fn,
>> > + inform (cloc, "%s %<%D(%T)%> ", 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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-19 Thread Christophe Lyon
On Thu, 18 Apr 2019 at 18:25, Richard Sandiford
 wrote:
>
> Christophe Lyon  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)%> ", msg, fn,
> > + inform (cloc, "%s %<%D(%T, %T, %T)%> ", 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)%> ", msg, fn,
> > + inform (cloc, "%s %<%D(%T, %T)%> ", msg, fn,
> >   candidate->convs[0]->type,
> >   candidate->convs[1]->type);
> >else
> > - inform (cloc, "%s%<%D(%T)%> ", msg, fn,
> > + inform (cloc, "%s %<%D(%T)%> ", 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


Re: [PATCH] PR translation/90118 Missing space between words

2019-04-18 Thread Richard Sandiford
Christophe Lyon  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)%> ", msg, fn,
> + inform (cloc, "%s %<%D(%T, %T, %T)%> ", 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)%> ", msg, fn,
> + inform (cloc, "%s %<%D(%T, %T)%> ", msg, fn,
>   candidate->convs[0]->type,
>   candidate->convs[1]->type);
>else
> - inform (cloc, "%s%<%D(%T)%> ", msg, fn,
> + inform (cloc, "%s %<%D(%T)%> ", 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