Hi Chris, In the version before: report.addf(__("Package %i"), _package->get_number()); I have used the conditional gettext() macro which generates localized table column names for HTML-reports and generate original column names for CSV-reports: /* Conditional gettext. We need original strings for CSV. */ #define __(STRING) \ ((report.get_type() == REPORT_CSV) ? (STRING) : gettext(STRING)) And in your version we have lost the translation for HTML-reports. I don't know whether we need to translate words "CPU", "Core" and "Package" to different languages in column names in HTML report. It should be your decision. ;-) Ferron, Chris E wrote: > Igor, Here is what I am thinking ? > --- > diff --git a/src/cpu/cpu.cpp b/src/cpu/cpu.cpp > index 18cbffc..9767d52 100644 > --- a/src/cpu/cpu.cpp > +++ b/src/cpu/cpu.cpp > @@ -39,6 +39,13 @@ > #include "../report/report.h" > #include "../report/report-maker.h" > > +#define gettext_noop(String) String > +#define N_(String) gettext_noop(String) > +/*TRANSLATORS: This is "package" as in a processor Package */ > +#define PACKAGE_TYPE N_("Package") > +/*TRANSLATORS: This is "Core" as in a processor Core */ > +#define CORE_TYPE N_("Core") > + > static class abstract_cpu system_level; > > vector all_cpus; > @@ -87,7 +94,7 @@ static class abstract_cpu * new_package(int package, > int cpu, char * vendor, int > ret = new class cpu_package; > > ret->set_number(package, cpu); > - ret->set_type("Package"); > + ret->set_type(PACKAGE_TYPE); > ret->childcount = 0; > > sprintf(packagename, _("cpu package %i"), cpu); > @@ -123,7 +130,7 @@ static class abstract_cpu * new_core(int core, int > cpu, char * vendor, int famil > ret = new class cpu_core; > ret->set_number(core, cpu); > ret->childcount = 0; > - ret->set_type("Core"); > + ret->set_type(CORE_TYPE); > > return ret; > } > diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h > index 4480b11..d16303f 100644 > --- a/src/cpu/cpu.h > +++ b/src/cpu/cpu.h > @@ -88,7 +88,7 @@ public: > uint64_t total_stamp; > int number; > int childcount; > - const char* name; > + const char* type; > bool idle, old_idle; > uint64_t current_frequency; > uint64_t effective_frequency; > @@ -103,9 +103,9 @@ public: > > > void set_number(int _number, int cpu) {this->number = > _number; this->first_cpu = cpu;}; > - void set_type(const char* _name) {this->name = _name;}; > + void set_type(const char* _type) {this->type = _type;}; > int get_number(void) { return number; }; > - const char* get_type(void) { return name; }; > + const char* get_type(void) { return type; }; > > virtual void measurement_start(void); > virtual void measurement_end(void); > > > On Thu, Oct 25, 2012 at 10:39 AM, Ferron, Chris E > wrote: >> On Thu, Oct 25, 2012 at 1:55 AM, Igor Zhbanov wrote: >>> Hi Chris, >>> >>> It seems that there is a problem with internationalization with the last >>> commit. >>> Please look here: >>> - report.addf(__("Package %i"), _package->get_number()); >>> + report.addf(__("%s %i"), _package->get_type(), _package->get_number()); >>> >>> I have introduced the macro __() /* Double underscore */ to be a conditional >>> variant >>> of classical _() /* Single underscore */ in the report refactoring commit. >>> >>> Usual underscore macro calls the gettext() function. And we need some sort >>> of conditional >>> gettext() -- we want to print untranslated strings to CSV-report. (Of >>> course, we could simply >>> set English locale for CSV-reports. But this will look strange.) >>> >>> So I have introduced the double underscore macro which calls gettext() for >>> HTML-reports and >>> return original strings for CSV. >>> >>> After your patch the gettext will try to translate the string "%s %i" and >>> not the "Package %i", >>> so we have lost the internationalization here. >> You are correct, an oversight on my part by including the macro >> without consideration. >>> We could fix it by internationalizing the "type" field in the class: >>> ret->set_type(__("Package")); >> Yes you are right this is bad. I think the best solution for this is >> to define them as and use gettext noop. I will add a translation >> comment to them so translators have context. >> #define gettext_noop(String) String >> #define N_(String) gettext_noop(String) >> #define PACKAGE_TYPE N_("Package") >> #define CORE_TYPE N_("Core") >> >>> But I'm not sure that it is good to have the type depending on current >>> locale (especially >>> if in some future there will be need to compare the value of this field by >>> strcmp()). >> My personal opinion is to remove all localization from reports, but >> enough people have voiced that they find it valuable to be able to >> generate reported in the language of there local. >> >> As such I find there are several issues with internationalization when >> it comes to reports. When we did internationalization it was supposed >> to be for the UI. We then started thinking and planning to include in >> the HTML report the ability to switch from local to engineering >> (engineering being English) to solve most of the issues. Also it been >> considered that CSV not be localized, as it should be considered >> machine language and not meant to be necessarily human readable(which >> is a good feature to the reporting work already done). Thus why as for >> CSV standard discussions I only asked that "Office" type products be >> able to parse them. >> >>> What you think? >>> >>> P.S. Why you not sending patches to mailing list for discussion? ;-) >> I do for large changes, but usually do not for quick fixes. >> >> Thank you for pointing this out, I will fix this. -- Best regards, Igor Zhbanov, Technical Leader, phone: +7 (495) 797 25 00 ext 3806 e-mail: i.zhbanov(a)samsung.com Mobile group, Moscow R&D center, Samsung Electronics 12 Dvintsev street, building 1 127018, Moscow, Russian Federation