Fix i18n in warn_spec_missing_attributes (PR 83871)

On Tue, Feb 27, 2018 at 05:52:03PM -0700, Martin Sebor wrote:
> > This is broken for multiple reasons:
> > 1) it should be inform_n rather than inform
> > 2) you really can't do what you're doing for translations;
> >    G_(...) marks the string for translations, but what actually is
> >    translated is not that string, but rather what is passed to inform,
> >    i.e. str.c_str (), so it will be likely never translated
> > 3) as others have mentioned, the #include <string> you are doing is
> >    wrong
> > 4) I don't see justification to use std::string here
> > 
> > What you IMHO should use instead is use
> >   pretty_printer str;
> > instead, and the pp_* APIs to add stuff in there, including
> > pp_begin_quote (&str, pp_show_color (global_dc->printer))
> > and
> > pp_end_quote (&str, pp_show_color (global_dc->printer))
> > when you want to add what %< or %> expand to,
> > and finally
> >   inform_n (DECL_SOURCE_LOCATION (tmpl), nattrs,
> > 	    "missing primary template attribute %s",
> > 	    "missing primary template attributes %s",
> > 	    pp_formatted_text (&str));
> > That way it should be properly translatable.
> 
> Using inform_n() would not be correct here.  What's being
> translated is one of exactly two forms: singular and plural.
> It doesn't matter how many things the plural form refers to
> because the number doesn't appear in the message.  Let's ask
> Google to translate the message above to a language with more
> than two plural forms, such as Czech:
> 
> there are missing attributes:
> https://translate.google.com/?tl=cs#auto/cs/there%20are%20missing%20attributes
> 
> vs there are 5 missing attributes:
> https://translate.google.com/?tl=cs#auto/cs/there%20are%205%20missing%20attributes
> 
> Only the first form is correct when the exact number isn't
> mentioned.

I stand corrected on 1) (though wonder if there are locales
which have different plurals based on the count in the list).

> There are many places in the C++ front-end where a string
> enclosed in G_() is assigned to a pointer and later used
> in a diagnostic call.  Is there something different about
> the usage I introduced that makes it unsuitable for
> translation?

G_() does what it is designed for, collects a string for
exgettext extraction into *.pot file.  For most of the
diagnostic calls we have arranged automatic extractions of
the string literals passed directly to the functions, so we don't need
to type it in most places, just where there is a conditional or
the string is elsewhere.  The problem in your code is that
you mark using G_() for translation one string literal,
e.g.
G_("missing primary template attributes ")
but then actually call inform with something different, like:
"missing primary template attributes %<malloc%>, %<alloc_size%>"
Functions inform will call will call gettext to translate that, but
as this second string is dynamic and never in the *.pot file, nobody
will translate it and so the inform will be always in English, even
when some translator translates everything he is provided.

And another advantage of using the %s appart from translatability is
that it is -Wformat-security compatible.

> std::string is used in a number of places in GCC.  Why does
> using it here need any special justification?

Outside of config/*/* (aarch64, arm, nvptx, sh backends only) where
the maintainers chose to use it, it is used just in a single file,
ipa-chkp.c which is going away soon, all the diagnostics uses the pretty
printers infrastructure instead.  More importantly, with the latter
you can do the begin/end quotes easily, with std::string you can't, unless
you just use pretty printers and convert that to std::string, at which point
it is certainly less efficient.

> Using the pretty printer as you suggest also sounds
> complicated to me and so prone to error but I will defer
> to Jason's opinion to decide if any changes are necessary.

Here it is in patch form, tested so far with:
make check-c++-all RUNTESTFLAGS="--target_board=unix\{-m32,-m64\} dg.exp='attr*.C Wmissing-attrib*'"
and waiting for bootstrap/regtest, ok for trunk if it passes?

2018-02-28  Jakub Jelinek  <jakub@redhat.com>

	PR c++/83871
	PR c++/83503
	* pt.c (INCLUDE_STRING): Remove define.
	(warn_spec_missing_attributes): Use pretty_printer instead of
	std::string.  Fix up inform call so that the list of attributes
	is in %s argument.



	Jakub

On Wed, Feb 28, 2018 at 5:26 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Feb 27, 2018 at 05:52:03PM -0700, Martin Sebor wrote:
>> > This is broken for multiple reasons:
>> > 1) it should be inform_n rather than inform
>> > 2) you really can't do what you're doing for translations;
>> >    G_(...) marks the string for translations, but what actually is
>> >    translated is not that string, but rather what is passed to inform,
>> >    i.e. str.c_str (), so it will be likely never translated
>> > 3) as others have mentioned, the #include <string> you are doing is
>> >    wrong
>> > 4) I don't see justification to use std::string here
>> >
>> > What you IMHO should use instead is use
>> >   pretty_printer str;
>> > instead, and the pp_* APIs to add stuff in there, including
>> > pp_begin_quote (&str, pp_show_color (global_dc->printer))
>> > and
>> > pp_end_quote (&str, pp_show_color (global_dc->printer))
>> > when you want to add what %< or %> expand to,
>> > and finally
>> >   inform_n (DECL_SOURCE_LOCATION (tmpl), nattrs,
>> >         "missing primary template attribute %s",
>> >         "missing primary template attributes %s",
>> >         pp_formatted_text (&str));
>> > That way it should be properly translatable.
>>
>> Using inform_n() would not be correct here.  What's being
>> translated is one of exactly two forms: singular and plural.
>> It doesn't matter how many things the plural form refers to
>> because the number doesn't appear in the message.  Let's ask
>> Google to translate the message above to a language with more
>> than two plural forms, such as Czech:
>>
>> there are missing attributes:
>> https://translate.google.com/?tl=cs#auto/cs/there%20are%20missing%20attributes
>>
>> vs there are 5 missing attributes:
>> https://translate.google.com/?tl=cs#auto/cs/there%20are%205%20missing%20attributes
>>
>> Only the first form is correct when the exact number isn't
>> mentioned.
>
> I stand corrected on 1) (though wonder if there are locales
> which have different plurals based on the count in the list).
>
>> There are many places in the C++ front-end where a string
>> enclosed in G_() is assigned to a pointer and later used
>> in a diagnostic call.  Is there something different about
>> the usage I introduced that makes it unsuitable for
>> translation?
>
> G_() does what it is designed for, collects a string for
> exgettext extraction into *.pot file.  For most of the
> diagnostic calls we have arranged automatic extractions of
> the string literals passed directly to the functions, so we don't need
> to type it in most places, just where there is a conditional or
> the string is elsewhere.  The problem in your code is that
> you mark using G_() for translation one string literal,
> e.g.
> G_("missing primary template attributes ")
> but then actually call inform with something different, like:
> "missing primary template attributes %<malloc%>, %<alloc_size%>"
> Functions inform will call will call gettext to translate that, but
> as this second string is dynamic and never in the *.pot file, nobody
> will translate it and so the inform will be always in English, even
> when some translator translates everything he is provided.
>
> And another advantage of using the %s appart from translatability is
> that it is -Wformat-security compatible.
>
>> std::string is used in a number of places in GCC.  Why does
>> using it here need any special justification?
>
> Outside of config/*/* (aarch64, arm, nvptx, sh backends only) where
> the maintainers chose to use it, it is used just in a single file,
> ipa-chkp.c which is going away soon, all the diagnostics uses the pretty
> printers infrastructure instead.  More importantly, with the latter
> you can do the begin/end quotes easily, with std::string you can't, unless
> you just use pretty printers and convert that to std::string, at which point
> it is certainly less efficient.
>
>> Using the pretty printer as you suggest also sounds
>> complicated to me and so prone to error but I will defer
>> to Jason's opinion to decide if any changes are necessary.
>
> Here it is in patch form, tested so far with:
> make check-c++-all RUNTESTFLAGS="--target_board=unix\{-m32,-m64\} dg.exp='attr*.C Wmissing-attrib*'"
> and waiting for bootstrap/regtest, ok for trunk if it passes?

Yes.  (So never mind the _ suggestion in the other thread).

Jason