Patchwork Clean up pretty printers [15/n]

login
register
mail settings
Submitter Gabriel Dos Reis
Date Aug. 25, 2013, 5:54 a.m.
Message ID <8738py71tx.fsf@euclid.axiomatics.org>
Download mbox | patch
Permalink /patch/269681/
State New
Headers show

Comments

Gabriel Dos Reis - Aug. 25, 2013, 5:54 a.m.
Instead of defining the same macro several times in different
translation units, we can just make it a function and use it where
needed.

Tested on an x86_64-suse-linux.  Applied to mainline.

-- Gaby

2013-08-25  Gabriel Dos Reis  <gdr@integrable-solutions.net>
c-family/
	* c-pretty-print.h (c_pretty_printer::translate_string): Declare.
	* c-pretty-print.c (M_): Remove.
	(c_pretty_printer::translate_string): Define.
	(pp_c_type_specifier): Use it.
	(pp_c_primary_expression): Likewise.
	(pp_c_expression): Likewise.

cp/
	* cxx-pretty-print.c (M_): Remove.
	(pp_cxx_unqualified_id): Use translate_string instead of M_.
	(pp_cxx_canonical_template_parameter): Likewise.
Joseph S. Myers - Aug. 25, 2013, 3:14 p.m.
On Sun, 25 Aug 2013, Gabriel Dos Reis wrote:

> Instead of defining the same macro several times in different
> translation units, we can just make it a function and use it where
> needed.

Have you made sure that po/exgettext still extracts the relevant messages 
for translation (I'm not sure how good it is at identifying C++ functions 
with arguments called gmsgid, or how good xgettext then is at identifying 
relevant C++ function calls)?  In particular, even if other cases get 
identified properly, conditional expressions such as

> @@ -379,15 +375,15 @@
>  	      switch (code)
>  		{
>  		case INTEGER_TYPE:
> -		  pp_string (pp, (TYPE_UNSIGNED (t)
> -				  ? M_("<unnamed-unsigned:")
> -				  : M_("<unnamed-signed:")));
> +		  pp->translate_string (TYPE_UNSIGNED (t)
> +                                        ? "<unnamed-unsigned:"
> +                                        : "<unnamed-signed:");

may need each case of the conditional expression to be marked for 
extraction for translation, or to be separated into two separate calls 
using "if" (we've had that issue before with conditional expressions in 
diagnostics).
Gabriel Dos Reis - Aug. 25, 2013, 3:37 p.m.
"Joseph S. Myers" <joseph@codesourcery.com> writes:

| On Sun, 25 Aug 2013, Gabriel Dos Reis wrote:
| 
| > Instead of defining the same macro several times in different
| > translation units, we can just make it a function and use it where
| > needed.
| 
| Have you made sure that po/exgettext still extracts the relevant messages 
| for translation (I'm not sure how good it is at identifying C++ functions 
| with arguments called gmsgid, or how good xgettext then is at identifying 
| relevant C++ function calls)?  In particular, even if other cases get 
| identified properly, conditional expressions such as

I trusted xgettext documentation that says that it handles C++ input
source files. 

Looking at the documentation, I notice this:

       By  default  the  language  is guessed depending on the input
       file name extension.

I don't know whether after the switch C++ xgettext is being invoked
explicitly with -C or --c++, or whether we are still relying on xgettext
to pick the language based on the file extension.  This is probably
another reason why we might want to rename files to use apppropriate
extensions.  In the meantime, we might want to explicitly specify the
language for the input source file.

po/exgettext only looks whether the parameter name ends with 'msgid'.

| 
| > @@ -379,15 +375,15 @@
| >  	      switch (code)
| >  		{
| >  		case INTEGER_TYPE:
| > -		  pp_string (pp, (TYPE_UNSIGNED (t)
| > -				  ? M_("<unnamed-unsigned:")
| > -				  : M_("<unnamed-signed:")));
| > +		  pp->translate_string (TYPE_UNSIGNED (t)
| > +                                        ? "<unnamed-unsigned:"
| > +                                        : "<unnamed-signed:");
| 
| may need each case of the conditional expression to be marked for 
| extraction for translation, or to be separated into two separate calls 
| using "if" (we've had that issue before with conditional expressions in 
| diagnostics).

Hmm, why would that be needed now, and not before?
(not that I am found of the conditional, but only by curiosity.)

-- Gaby
Joseph S. Myers - Aug. 25, 2013, 5:05 p.m.
On Sun, 25 Aug 2013, Gabriel Dos Reis wrote:

> I don't know whether after the switch C++ xgettext is being invoked
> explicitly with -C or --c++, or whether we are still relying on xgettext
> to pick the language based on the file extension.  This is probably
> another reason why we might want to rename files to use apppropriate
> extensions.  In the meantime, we might want to explicitly specify the
> language for the input source file.
> 
> po/exgettext only looks whether the parameter name ends with 'msgid'.

And uses --language=c, --language=c++ and --language=GCC-source depending 
on the source files and functions in question.  The issue is both whether 
the awk code properly handles C++ function declarations to identify the 
parameter, and whether whatever way it passed the identified function name 
to xgettext (if it does identify the function) results in xgettext (with 
whatever language it uses) handling the call correctly.

> | > @@ -379,15 +375,15 @@
> | >  	      switch (code)
> | >  		{
> | >  		case INTEGER_TYPE:
> | > -		  pp_string (pp, (TYPE_UNSIGNED (t)
> | > -				  ? M_("<unnamed-unsigned:")
> | > -				  : M_("<unnamed-signed:")));
> | > +		  pp->translate_string (TYPE_UNSIGNED (t)
> | > +                                        ? "<unnamed-unsigned:"
> | > +                                        : "<unnamed-signed:");
> | 
> | may need each case of the conditional expression to be marked for 
> | extraction for translation, or to be separated into two separate calls 
> | using "if" (we've had that issue before with conditional expressions in 
> | diagnostics).
> 
> Hmm, why would that be needed now, and not before?
> (not that I am found of the conditional, but only by curiosity.)

Previously, each string was inside a separate call to M_() - the strings 
themselves were the msgid parameters.  Now, the msgid parameter is not a 
single string but a more complicated expression and xgettext may not 
handle such expressions properly the way it handles having just a single 
string as parameter.
Gabriel Dos Reis - Aug. 25, 2013, 6:11 p.m.
"Joseph S. Myers" <joseph@codesourcery.com> writes:

[...]

| > | > @@ -379,15 +375,15 @@
| > | >  	      switch (code)
| > | >  		{
| > | >  		case INTEGER_TYPE:
| > | > -		  pp_string (pp, (TYPE_UNSIGNED (t)
| > | > -				  ? M_("<unnamed-unsigned:")
| > | > -				  : M_("<unnamed-signed:")));
| > | > +		  pp->translate_string (TYPE_UNSIGNED (t)
| > | > +                                        ? "<unnamed-unsigned:"
| > | > +                                        : "<unnamed-signed:");
| > | 
| > | may need each case of the conditional expression to be marked for 
| > | extraction for translation, or to be separated into two separate calls 
| > | using "if" (we've had that issue before with conditional expressions in 
| > | diagnostics).
| > 
| > Hmm, why would that be needed now, and not before?
| > (not that I am found of the conditional, but only by curiosity.)
| 
| Previously, each string was inside a separate call to M_() - the strings 
| themselves were the msgid parameters.  Now, the msgid parameter is not a 
| single string but a more complicated expression and xgettext may not 
| handle such expressions properly the way it handles having just a single 
| string as parameter.

OK, thanks the explanation.

Do you think the same issue arise with diagnostic_set_info,
diagnostic_append_note?

-- Gaby
Joseph S. Myers - Aug. 25, 2013, 7:59 p.m.
On Sun, 25 Aug 2013, Gabriel Dos Reis wrote:

> | Previously, each string was inside a separate call to M_() - the strings 
> | themselves were the msgid parameters.  Now, the msgid parameter is not a 
> | single string but a more complicated expression and xgettext may not 
> | handle such expressions properly the way it handles having just a single 
> | string as parameter.
> 
> OK, thanks the explanation.
> 
> Do you think the same issue arise with diagnostic_set_info,
> diagnostic_append_note?

I don't see any problematic calls to those functions with this sort of 
conditional expression.

Patch

Index: c-family/c-pretty-print.c
===================================================================
--- c-family/c-pretty-print.c	(revision 201973)
+++ c-family/c-pretty-print.c	(working copy)
@@ -29,10 +29,6 @@ 
 #include "tree-iterator.h"
 #include "diagnostic.h"
 
-/* Translate if being used for diagnostics, but not for dump files or
-   __PRETTY_FUNCTION.  */
-#define M_(msgid) (pp_translate_identifiers (pp) ? _(msgid) : (msgid))
-
 /* The pretty-printer code is primarily designed to closely follow
    (GNU) C and C++ grammars.  That is to be contrasted with spaghetti
    codes we used to have in the past.  Following a structured
@@ -341,7 +337,7 @@ 
   switch (code)
     {
     case ERROR_MARK:
-      pp_c_ws_string (pp, M_("<type-error>"));
+      pp->translate_string ("<type-error>");
       break;
 
     case IDENTIFIER_NODE:
@@ -379,15 +375,15 @@ 
 	      switch (code)
 		{
 		case INTEGER_TYPE:
-		  pp_string (pp, (TYPE_UNSIGNED (t)
-				  ? M_("<unnamed-unsigned:")
-				  : M_("<unnamed-signed:")));
+		  pp->translate_string (TYPE_UNSIGNED (t)
+                                        ? "<unnamed-unsigned:"
+                                        : "<unnamed-signed:");
 		  break;
 		case REAL_TYPE:
-		  pp_string (pp, M_("<unnamed-float:"));
+		  pp->translate_string ("<unnamed-float:");
 		  break;
 		case FIXED_POINT_TYPE:
-		  pp_string (pp, M_("<unnamed-fixed:"));
+		  pp->translate_string ("<unnamed-fixed:");
 		  break;
 		default:
 		  gcc_unreachable ();
@@ -402,7 +398,7 @@ 
       if (DECL_NAME (t))
 	pp_id_expression (pp, t);
       else
-	pp_c_ws_string (pp, M_("<typedef-error>"));
+	pp->translate_string ("<typedef-error>");
       break;
 
     case UNION_TYPE:
@@ -415,12 +411,12 @@ 
       else if (code == ENUMERAL_TYPE)
 	pp_c_ws_string (pp, "enum");
       else
-	pp_c_ws_string (pp, M_("<tag-error>"));
+	pp->translate_string ("<tag-error>");
 
       if (TYPE_NAME (t))
 	pp_id_expression (pp, TYPE_NAME (t));
       else
-	pp_c_ws_string (pp, M_("<anonymous>"));
+	pp->translate_string ("<anonymous>");
       break;
 
     default:
@@ -1187,6 +1183,15 @@ 
   pp->padding = pp_before;
 }
 
+void
+c_pretty_printer::translate_string (const char *gmsgid)
+{
+  if (pp_translate_identifiers (this))
+    pp_c_ws_string (this, _(gmsgid));
+  else
+    pp_c_ws_string (this, gmsgid);
+}
+
 /* Pretty-print an IDENTIFIER_NODE, which may contain UTF-8 sequences
    that need converting to the locale encoding, preceded by whitespace
    is necessary.  */
@@ -1225,11 +1230,11 @@ 
       break;
 
     case ERROR_MARK:
-      pp_c_ws_string (pp, M_("<erroneous-expression>"));
+      pp->translate_string ("<erroneous-expression>");
       break;
 
     case RESULT_DECL:
-      pp_c_ws_string (pp, M_("<return-value>"));
+      pp->translate_string ("<return-value>");
       break;
 
     case INTEGER_CST:
@@ -2155,7 +2160,7 @@ 
 	  && !DECL_ARTIFICIAL (SSA_NAME_VAR (e)))
 	pp_c_expression (pp, SSA_NAME_VAR (e));
       else
-	pp_c_ws_string (pp, M_("<unknown>"));
+	pp->translate_string ("<unknown>");
       break;
 
     case POSTINCREMENT_EXPR:
Index: c-family/c-pretty-print.h
===================================================================
--- c-family/c-pretty-print.h	(revision 201973)
+++ c-family/c-pretty-print.h	(working copy)
@@ -51,6 +51,9 @@ 
 {
   c_pretty_printer ();
 
+  // Format string, possibly translated.
+  void translate_string (const char *);
+
   virtual void constant (tree);
   virtual void id_expression (tree);
   /* Points to the first element of an array of offset-list.
Index: cp/cxx-pretty-print.c
===================================================================
--- cp/cxx-pretty-print.c	(revision 201973)
+++ cp/cxx-pretty-print.c	(working copy)
@@ -27,10 +27,6 @@ 
 #include "cxx-pretty-print.h"
 #include "tree-pretty-print.h"
 
-/* Translate if being used for diagnostics, but not for dump files or
-   __PRETTY_FUNCTION.  */
-#define M_(msgid) (pp_translate_identifiers (pp) ? _(msgid) : (msgid))
-
 static void pp_cxx_unqualified_id (cxx_pretty_printer *, tree);
 static void pp_cxx_nested_name_specifier (cxx_pretty_printer *, tree);
 static void pp_cxx_qualified_id (cxx_pretty_printer *, tree);
@@ -149,7 +145,7 @@ 
   switch (code)
     {
     case RESULT_DECL:
-      pp_cxx_ws_string (pp, M_("<return-value>"));
+      pp->translate_string ("<return-value>");
       break;
 
     case OVERLOAD:
@@ -168,7 +164,7 @@ 
 
     case IDENTIFIER_NODE:
       if (t == NULL)
-	pp_cxx_ws_string (pp, M_("<unnamed>"));
+	pp->translate_string ("<unnamed>");
       else if (IDENTIFIER_TYPENAME_P (t))
 	pp_cxx_conversion_function_id (pp, t);
       else
@@ -2154,7 +2150,7 @@ 
     parm = TEMPLATE_TYPE_PARM_INDEX (parm);
 
   pp_cxx_begin_template_argument_list (pp);
-  pp_cxx_ws_string (pp, M_("template-parameter-"));
+  pp->translate_string ("template-parameter-");
   pp_wide_integer (pp, TEMPLATE_PARM_LEVEL (parm));
   pp_minus (pp);
   pp_wide_integer (pp, TEMPLATE_PARM_IDX (parm) + 1);