| Message ID | CAKdteOaCsYMn87U8ZkKFHn-swGf46-R-KTyw4+BK14J1RB+TDQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR translation/90118 Missing space between words | expand |
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
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
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
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
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
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
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
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
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))