On Wed, Feb 28, 2018 at 5:26 AM, Jakub Jelinek <ja...@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