Re: Patch that fix PR80188

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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.