Patch that fix PR80188

Patch that fix PR80188

[nick]

Greetings,

I don't have write access so can someone commit this bug fix as it fixes,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188. 

Author: Nicholas Krause <xerofoify@gmail.com>
Date:   Fri Sep 29 11:39:46 2017 -0400

    This patch fixes, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188
    which reports that the char* pointer reason is not being translated
    properly when the error message from the function,
    maybe_complain_about_tail_call arises. Fix it by wrapping it in the
    N_ macro to translate to the proper language of the user. No new
    test cases are required due to the triviality of the bug.

diff --git a/gcc/calls.c b/gcc/calls.c
index 6bd025ed197..cfdd6b2cf6b 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1516,7 +1516,7 @@ maybe_complain_about_tail_call (tree call_expr, const char *reason)
   if (!CALL_EXPR_MUST_TAIL_CALL (call_expr))
     return;
 
-  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", reason);
+  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", N_(reason));
 }
 
 /* Fill in ARGS_SIZE and ARGS array based on the parameters found in

Thanks,

Nick 

Re: Patch that fix PR80188

[Bernd Edlinger]

 > Greetings,
 >
 > I don't have write access so can someone commit this bug fix as it
 > fixes,
 > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188.
 >
 > Author: Nicholas Krause <xerofoify@gmail.com>
 > Date:   Fri Sep 29 11:39:46 2017 -0400
 >
 >    This patch fixes, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188
 >    which reports that the char* pointer reason is not being translated
 >    properly when the error message from the function,
 >    maybe_complain_about_tail_call arises. Fix it by wrapping it in the
 >    N_ macro to translate to the proper language of the user. No new
 >    test cases are required due to the triviality of the bug.
 >
 > diff --git a/gcc/calls.c b/gcc/calls.c
 > index 6bd025ed197..cfdd6b2cf6b 100644
 > --- a/gcc/calls.c
 > +++ b/gcc/calls.c
 > @@ -1516,7 +1516,7 @@ maybe_complain_about_tail_call (tree call_expr, 
const char *reason)
 >    if (!CALL_EXPR_MUST_TAIL_CALL (call_expr))
 >      return;
 >
 > -  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", reason);
 > +  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", 
N_(reason));
 > }
 >
 > /* Fill in ARGS_SIZE and ARGS array based on the parameters found in
 >
 > Thanks,
 >
 > Nick

No, this does obviously not fix the problem.

The main problem is that po/gcc.pot does contain the "cannot tail-call"
string but not the various reasons for it, so the translators have
noting to translate.

You should wrap all strings that need to be translated in N_,
and where you do use N_ you should use _(reason).
So that make -C gcc gcc.pot picks them up when the gcc.pot is created,
which is only done on request, but it would be good to check
that the gcc.pot file looks right with your patch at least.

But most importantly a patch like this is worthless when it was not
tested, so the minimum is you have to state that you did bootstrap with
your patch and the test suite did not produce any new failures
that were not there without your patch.


Bernd.

Re: Patch that fix PR80188

[nick]

On 2017-09-29 01:48 PM, Bernd Edlinger wrote:
>  > Greetings,
>  >
>  > I don't have write access so can someone commit this bug fix as it
>  > fixes,
>  > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188.
>  >
>  > Author: Nicholas Krause <xerofoify@gmail.com>
>  > Date:   Fri Sep 29 11:39:46 2017 -0400
>  >
>  >    This patch fixes, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188
>  >    which reports that the char* pointer reason is not being translated
>  >    properly when the error message from the function,
>  >    maybe_complain_about_tail_call arises. Fix it by wrapping it in the
>  >    N_ macro to translate to the proper language of the user. No new
>  >    test cases are required due to the triviality of the bug.
>  >
>  > diff --git a/gcc/calls.c b/gcc/calls.c
>  > index 6bd025ed197..cfdd6b2cf6b 100644
>  > --- a/gcc/calls.c
>  > +++ b/gcc/calls.c
>  > @@ -1516,7 +1516,7 @@ maybe_complain_about_tail_call (tree call_expr, 
> const char *reason)
>  >    if (!CALL_EXPR_MUST_TAIL_CALL (call_expr))
>  >      return;
>  >
>  > -  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", reason);
>  > +  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", 
> N_(reason));
>  > }
>  >
>  > /* Fill in ARGS_SIZE and ARGS array based on the parameters found in
>  >
>  > Thanks,
>  >
>  > Nick
> 
> No, this does obviously not fix the problem.
> 
> The main problem is that po/gcc.pot does contain the "cannot tail-call"
> string but not the various reasons for it, so the translators have
> noting to translate.
> 
> You should wrap all strings that need to be translated in N_,
> and where you do use N_ you should use _(reason).
> So that make -C gcc gcc.pot picks them up when the gcc.pot is created,
> which is only done on request, but it would be good to check
> that the gcc.pot file looks right with your patch at least.
> 

So I understand correctly the gcc.pot is used for something and that the
cannot tail call but not the various reasons for it. So this N_ marco 
is a way to get debugging or symbol information or something more like:

error_at (EXPR_LOCATION (call_expr),N_("cannot tail-call: %s"),

gcc.pot for that line is: 
#: calls.c:1516
â–¸ prev-zlib/                   |16905 #, gcc-internal-format, gfc-internal-format
â–¸ stage1-fixincludes/          |16906 msgid "cannot tail-call: %s"
â–¸ stage1-gcc/                  |16907 msgstr ""

This seems wrong to me but I am new so double checking would be nice. Or our to talking
about all lines in gcc.pot requiring something similar? I am a bit confused by is it
just this area or all of the output that needs fixing in gcc.pot?
> But most importantly a patch like this is worthless when it was not
> tested, so the minimum is you have to state that you did bootstrap with
> your patch and the test suite did not produce any new failures
> that were not there without your patch.
> 
> 
I ran the test suite and got no known new failures. I assumed that I didn't need
to report that but if so that's fine. This is something I always do if possible.
Thanks for the quick reply, 
Nick  
> Bernd.
> 

Re: Patch that fix PR80188

[Bernd Edlinger]

On 09/29/17 20:22, nick wrote:
> 
> 
> On 2017-09-29 01:48 PM, Bernd Edlinger wrote:
>>   > Greetings,
>>   >
>>   > I don't have write access so can someone commit this bug fix as it
>>   > fixes,
>>   > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188.
>>   >
>>   > Author: Nicholas Krause <xerofoify@gmail.com>
>>   > Date:   Fri Sep 29 11:39:46 2017 -0400
>>   >
>>   >    This patch fixes, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80188
>>   >    which reports that the char* pointer reason is not being translated
>>   >    properly when the error message from the function,
>>   >    maybe_complain_about_tail_call arises. Fix it by wrapping it in the
>>   >    N_ macro to translate to the proper language of the user. No new
>>   >    test cases are required due to the triviality of the bug.
>>   >

BTW: the change log is not in GNU style. Please re-word.

>>   > diff --git a/gcc/calls.c b/gcc/calls.c
>>   > index 6bd025ed197..cfdd6b2cf6b 100644
>>   > --- a/gcc/calls.c
>>   > +++ b/gcc/calls.c
>>   > @@ -1516,7 +1516,7 @@ maybe_complain_about_tail_call (tree call_expr,
>> const char *reason)
>>   >    if (!CALL_EXPR_MUST_TAIL_CALL (call_expr))
>>   >      return;
>>   >
>>   > -  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", reason);
>>   > +  error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s",
>> N_(reason));
>>   > }
>>   >
>>   > /* Fill in ARGS_SIZE and ARGS array based on the parameters found in
>>   >
>>   > Thanks,
>>   >
>>   > Nick
>>
>> No, this does obviously not fix the problem.
>>
>> The main problem is that po/gcc.pot does contain the "cannot tail-call"
>> string but not the various reasons for it, so the translators have
>> noting to translate.
>>
>> You should wrap all strings that need to be translated in N_,
>> and where you do use N_ you should use _(reason).
>> So that make -C gcc gcc.pot picks them up when the gcc.pot is created,
>> which is only done on request, but it would be good to check
>> that the gcc.pot file looks right with your patch at least.
>>
> 
> So I understand correctly the gcc.pot is used for something and that the
> cannot tail call but not the various reasons for it. So this N_ marco
> is a way to get debugging or symbol information or something more like:
> 

Yes, I don't know all the details, but I think from time to time
some one runs a script that extracts all english strings from the gcc
sources.  The result is in gcc.pot, and as you can see it already
contains the string "cannot tail-call: %s" because the used tool
knows what error_at does.  But it does not know what
maybe_complain_about_tail_call does.  Therefore gcc.pot misses the
string "a callee-copied argument is stored in the current function's frame".

Then a lot of people go over the strings and translate to lots of
different languages.  Result is checked in SVN and used at runtime
by the _() function, which is a data base lookup, while N_() does
only annotate the string, and expands to nothing at runtime.

> error_at (EXPR_LOCATION (call_expr),N_("cannot tail-call: %s"),
> 

Please do not fix something that is not broken.
error_at does internally translate the first argument.
But I believe that is not the case for the format arguments.
so I think reason needs to be passed thru _().

> gcc.pot for that line is:
> #: calls.c:1516
> ▸ prev-zlib/                   |16905 #, gcc-internal-format, gfc-internal-format
> ▸ stage1-fixincludes/          |16906 msgid "cannot tail-call: %s"
> ▸ stage1-gcc/                  |16907 msgstr ""
> 
> This seems wrong to me but I am new so double checking would be nice. Or our to talking
> about all lines in gcc.pot requiring something similar? I am a bit confused by is it
> just this area or all of the output that needs fixing in gcc.pot?

I think this is only about the way how the cannot tail-call warning
is formatted.

>> But most importantly a patch like this is worthless when it was not
>> tested, so the minimum is you have to state that you did bootstrap with
>> your patch and the test suite did not produce any new failures
>> that were not there without your patch.
>>
>>
> I ran the test suite and got no known new failures. I assumed that I didn't need
> to report that but if so that's fine. This is something I always do if possible.
> Thanks for the quick reply,
> Nick

If you look at a few messages here, and you will see always a line
like "patch was boot-strapped and regression tested on
x86_64-pc-linux-gnu, is it OK for trunk?"

That's kind of required, otherwise we do not know what you have
actually tested.


Bernd.