Re: [PATCH] Fix i18n in warn_spec_missing_attributes (PR 83871)

[Jason Merrill <jason at redhat dot com>]

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