public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [RFC PATCH] avoid printing type suffix with %E
@ 2016-10-26 16:37 Martin Sebor
  2016-10-26 20:46 ` Joseph Myers
  2016-11-16 18:34 ` [RFC PATCH] avoid printing type suffix with %E Jeff Law
  0 siblings, 2 replies; 8+ messages in thread
From: Martin Sebor @ 2016-10-26 16:37 UTC (permalink / raw)
  To: Gcc Patch List; +Cc: David Malcolm

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

When formatting an integer constant using the %E directive GCC
includes a suffix that indicates its type.  This can perhaps be
useful in some situations but in my experience it's distracting
and gets in the way when writing tests.

Here's an example:

   $ cat b.c && gcc b.c
   constexpr __SIZE_TYPE__ x = 2;

   enum E: bool { e = x };
   b.c:3:20: error: enumerator value 2ul is outside the range of 
underlying type ‘bool’
    enum E: bool { e = x };
                       ^

Notice the "2ul" in the error message.

As far as I can tell, Clang avoids printing the suffix and I think
it would be nice if the GCC pretty printer made it possible to avoid
it as well.

The attached patch implements one such approach by having the pretty
printer recognize the space format flag to suppress the type suffix,
so "%E" still prints the suffix but "% E" does not.  I did this to
preserve the existing output but I think it would be nicer to avoid
printing the suffix with %E and treat (for instance) the pound sign
as a request to add the suffix.  I have tested the attached patch
but not the alternative.

Does anyone have any comments/suggestions for which of the two
approaches would be preferable (or what I may have missed here)?
I CC David as the diagnostic maintainer.

Thanks
Martin


[-- Attachment #2: gcc-pretty-print.diff --]
[-- Type: text/x-patch, Size: 6130 bytes --]

gcc/c/ChangeLog:
2016-10-26  Martin Sebor  <msebor@redhat.com>

	* c-objc-common.c (c_tree_printer::get_flag): Declare new function
	(c_tree_printer::set_flag): Same.

gcc/c-family/ChangeLog:
2016-10-26  Martin Sebor  <msebor@redhat.com>

	* c-pretty-print.c (c_tree_printer::get_flag): Define.
	(c_tree_printer::set_flag): Define.
	(pp_c_integer_constant): Don't print type suffix when space
	is set in flags.
	* c-pretty-print.h (pp_c_flag_space): Add enumerator.

gcc/cp/ChangeLog:
2016-10-26  Martin Sebor  <msebor@redhat.com>

	* error.c (cp_printer): Set space in flags.

gcc/ChangeLog:
2016-10-26  Martin Sebor  <msebor@redhat.com>

	* pretty-print.c (pp_format): Recognize space.
	* pretty-print.h (pretty_printer::get_flag): New function.
	 (pretty_printer::set_flag): Same.

diff --git a/gcc/c-family/c-pretty-print.c b/gcc/c-family/c-pretty-print.c
index 90428ca..7d9375d 100644
--- a/gcc/c-family/c-pretty-print.c
+++ b/gcc/c-family/c-pretty-print.c
@@ -292,6 +292,26 @@ pp_c_pointer (c_pretty_printer *pp, tree t)
     }
 }
 
+bool
+c_pretty_printer::get_flag (char flag)
+{
+  return ' ' == flag && (flags & pp_c_flag_space);
+}
+
+bool
+c_pretty_printer::set_flag (char flag, bool value)
+{
+  gcc_assert (flag == ' ');
+
+  bool previous = c_pretty_printer::get_flag (flag);
+  if (value)
+    flag |= pp_c_flag_space;
+  else
+    flag &= ~pp_c_flag_space;
+
+  return previous;
+}
+
 /* simple-type-specifier:
      type-specifier
 
@@ -926,24 +946,34 @@ pp_c_integer_constant (c_pretty_printer *pp, tree i)
       print_hex (wi, pp_buffer (pp)->digit_buffer);
       pp_string (pp, pp_buffer (pp)->digit_buffer);
     }
-  if (TYPE_UNSIGNED (type))
-    pp_character (pp, 'u');
-  if (type == long_integer_type_node || type == long_unsigned_type_node)
-    pp_character (pp, 'l');
-  else if (type == long_long_integer_type_node
-	   || type == long_long_unsigned_type_node)
-    pp_string (pp, "ll");
-  else for (idx = 0; idx < NUM_INT_N_ENTS; idx ++)
-    if (int_n_enabled_p[idx])
-      {
-	char buf[2+20];
-	if (type == int_n_trees[idx].signed_type
-	    || type == int_n_trees[idx].unsigned_type)
-	  {
-	    sprintf (buf, "I%d", int_n_data[idx].bitsize);
-	    pp_string (pp, buf);
-	  }
-      }
+
+  if (pp->get_flag (' '))
+    {
+      if (TYPE_UNSIGNED (type))
+	pp_character (pp, 'u');
+
+      if (type == long_integer_type_node || type == long_unsigned_type_node)
+	pp_character (pp, 'l');
+      else if (type == long_long_integer_type_node
+	       || type == long_long_unsigned_type_node)
+	pp_string (pp, "ll");
+      else
+	{
+	  for (idx = 0; idx < NUM_INT_N_ENTS; idx ++)
+	    {
+	      if (int_n_enabled_p[idx])
+		{
+		  char buf[2+20];
+		  if (type == int_n_trees[idx].signed_type
+		      || type == int_n_trees[idx].unsigned_type)
+		    {
+		      sprintf (buf, "I%d", int_n_data[idx].bitsize);
+		      pp_string (pp, buf);
+		    }
+		}
+	    }
+	}
+    }
 }
 
 /* Print out a CHARACTER literal.  */
diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h
index 253f192..10ad5d9 100644
--- a/gcc/c-family/c-pretty-print.h
+++ b/gcc/c-family/c-pretty-print.h
@@ -30,7 +30,8 @@ enum pp_c_pretty_print_flags
   {
      pp_c_flag_abstract = 1 << 1,
      pp_c_flag_gnu_v3 = 1 << 2,
-     pp_c_flag_last_bit = 3
+     pp_c_flag_space = 1 << 3,
+     pp_c_flag_last_bit = 4
   };
 
 
@@ -51,6 +52,10 @@ struct c_pretty_printer : pretty_printer
 {
   c_pretty_printer ();
 
+  /* Get and set the format flag.  */
+  virtual bool get_flag (char);
+  virtual bool set_flag (char, bool);
+
   // Format string, possibly translated.
   void translate_string (const char *);
 
diff --git a/gcc/c/c-objc-common.c b/gcc/c/c-objc-common.c
index 20dc024..1bd60cf 100644
--- a/gcc/c/c-objc-common.c
+++ b/gcc/c/c-objc-common.c
@@ -99,6 +99,12 @@ c_tree_printer (pretty_printer *pp, text_info *text, const char *spec,
 	text->set_location (0, DECL_SOURCE_LOCATION (t), true);
     }
 
+  if (' ' == *spec)
+    {
+      pp->set_flag (' ', true);
+      ++spec;
+    }
+
   switch (*spec)
     {
     case 'D':
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 20b20b4..d24ae26 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3539,6 +3539,12 @@ cp_printer (pretty_printer *pp, text_info *text, const char *spec,
   if (precision != 0 || wide)
     return false;
 
+  if (*spec == ' ')
+    {
+      pp->set_flag (' ');
+      ++spec;
+    }
+
   switch (*spec)
     {
     case 'A': result = args_to_string (next_tree, verbose);	break;
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index a39815e..93b7419 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -293,6 +293,11 @@ pp_indent (pretty_printer *pp)
    %.*s: a substring the length of which is specified by an argument
 	 integer.
    %Ns: likewise, but length specified as constant in the format string.
+
+   Flag 'w': wide integer (HOST_WIDE_INT).
+   Flag '+': always print a sign for integer and real constants.
+   Flag ' ': printer-specific.
+   Flag '#': printer-specific.
    Flag 'q': quote formatted text (must come immediately after '%').
 
    Arguments can be used sequentially, or through %N$ resp. *N$
@@ -425,12 +430,14 @@ pp_format (pretty_printer *pp, text_info *text)
       gcc_assert (argno < PP_NL_ARGMAX);
       gcc_assert (!formatters[argno]);
       formatters[argno] = &args[chunk];
+
+      /* Add known format flags.  */
       do
 	{
 	  obstack_1grow (&buffer->chunk_obstack, *p);
 	  p++;
 	}
-      while (strchr ("qwl+#", p[-1]));
+      while (strchr ("qwl+# ", p[-1]));
 
       if (p[-1] == '.')
 	{
diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h
index f49f35a..9d4386f 100644
--- a/gcc/pretty-print.h
+++ b/gcc/pretty-print.h
@@ -213,6 +213,13 @@ struct pretty_printer
 
   virtual ~pretty_printer ();
 
+  /* Get and set the format flag.  */
+  virtual bool get_flag (char) { return false; }
+  virtual bool set_flag (char, bool) { return false; }
+
+  /* Not virtual.  Provided for convenience.  */
+  bool set_flag (char flag) { return set_flag (flag, true); }
+
   /* Where we print external representation of ENTITY.  */
   output_buffer *buffer;
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E
  2016-10-26 16:37 [RFC PATCH] avoid printing type suffix with %E Martin Sebor
@ 2016-10-26 20:46 ` Joseph Myers
  2016-11-19 21:04   ` [RFC PATCH] avoid printing type suffix with %E (PR c/78165) Martin Sebor
  2016-11-16 18:34 ` [RFC PATCH] avoid printing type suffix with %E Jeff Law
  1 sibling, 1 reply; 8+ messages in thread
From: Joseph Myers @ 2016-10-26 20:46 UTC (permalink / raw)
  To: Martin Sebor; +Cc: Gcc Patch List, David Malcolm

On Wed, 26 Oct 2016, Martin Sebor wrote:

> The attached patch implements one such approach by having the pretty
> printer recognize the space format flag to suppress the type suffix,
> so "%E" still prints the suffix but "% E" does not.  I did this to
> preserve the existing output but I think it would be nicer to avoid
> printing the suffix with %E and treat (for instance) the pound sign
> as a request to add the suffix.  I have tested the attached patch
> but not the alternative.

I think printing the suffixes is a relic of %E being used to print full 
expressions.

It's established by now that printing expressions reconstructed from trees 
is a bad idea; we can get better results by having precise location ranges 
and underlining the relevant part of the source.  So if we could make sure 
nowhere is trying the use %E (or %qE, etc.) with expressions that might 
not be constants, where the type might be relevant, then we'd have 
confidence that stopping printing the suffix is safe.  But given the low 
quality of the reconstructed expressions, it's probably safe anyway.

(Most %qE uses are for identifiers not expressions.  If we give 
identifiers a different static type from "tree" - and certainly there 
isn't much reason for them to have the same type as expressions - then 
we'll need to change the format for either identifiers or expressions.)

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E
  2016-10-26 16:37 [RFC PATCH] avoid printing type suffix with %E Martin Sebor
  2016-10-26 20:46 ` Joseph Myers
@ 2016-11-16 18:34 ` Jeff Law
  2016-11-16 18:54   ` Martin Sebor
  1 sibling, 1 reply; 8+ messages in thread
From: Jeff Law @ 2016-11-16 18:34 UTC (permalink / raw)
  To: Martin Sebor, Gcc Patch List; +Cc: David Malcolm

On 10/26/2016 10:37 AM, Martin Sebor wrote:
> When formatting an integer constant using the %E directive GCC
> includes a suffix that indicates its type.  This can perhaps be
> useful in some situations but in my experience it's distracting
> and gets in the way when writing tests.
>
> Here's an example:
>
>   $ cat b.c && gcc b.c
>   constexpr __SIZE_TYPE__ x = 2;
>
>   enum E: bool { e = x };
>   b.c:3:20: error: enumerator value 2ul is outside the range of
> underlying type ‘bool’
>    enum E: bool { e = x };
>                       ^
>
> Notice the "2ul" in the error message.
>
> As far as I can tell, Clang avoids printing the suffix and I think
> it would be nice if the GCC pretty printer made it possible to avoid
> it as well.
>
> The attached patch implements one such approach by having the pretty
> printer recognize the space format flag to suppress the type suffix,
> so "%E" still prints the suffix but "% E" does not.  I did this to
> preserve the existing output but I think it would be nicer to avoid
> printing the suffix with %E and treat (for instance) the pound sign
> as a request to add the suffix.  I have tested the attached patch
> but not the alternative.
>
> Does anyone have any comments/suggestions for which of the two
> approaches would be preferable (or what I may have missed here)?
> I CC David as the diagnostic maintainer.
I'm having a hard time seeing how this is a significant issue, even when 
writing tests.

It also seems to me that relaying the type of the constant as a suffix 
would help in cases that aren't so obvious.

What am I missing?

Jeff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E
  2016-11-16 18:34 ` [RFC PATCH] avoid printing type suffix with %E Jeff Law
@ 2016-11-16 18:54   ` Martin Sebor
  0 siblings, 0 replies; 8+ messages in thread
From: Martin Sebor @ 2016-11-16 18:54 UTC (permalink / raw)
  To: Jeff Law, Gcc Patch List; +Cc: David Malcolm

On 11/16/2016 11:34 AM, Jeff Law wrote:
> On 10/26/2016 10:37 AM, Martin Sebor wrote:
>> When formatting an integer constant using the %E directive GCC
>> includes a suffix that indicates its type.  This can perhaps be
>> useful in some situations but in my experience it's distracting
>> and gets in the way when writing tests.
>>
>> Here's an example:
>>
>>   $ cat b.c && gcc b.c
>>   constexpr __SIZE_TYPE__ x = 2;
>>
>>   enum E: bool { e = x };
>>   b.c:3:20: error: enumerator value 2ul is outside the range of
>> underlying type ‘bool’
>>    enum E: bool { e = x };
>>                       ^
>>
>> Notice the "2ul" in the error message.
>>
>> As far as I can tell, Clang avoids printing the suffix and I think
>> it would be nice if the GCC pretty printer made it possible to avoid
>> it as well.
>>
>> The attached patch implements one such approach by having the pretty
>> printer recognize the space format flag to suppress the type suffix,
>> so "%E" still prints the suffix but "% E" does not.  I did this to
>> preserve the existing output but I think it would be nicer to avoid
>> printing the suffix with %E and treat (for instance) the pound sign
>> as a request to add the suffix.  I have tested the attached patch
>> but not the alternative.
>>
>> Does anyone have any comments/suggestions for which of the two
>> approaches would be preferable (or what I may have missed here)?
>> I CC David as the diagnostic maintainer.
> I'm having a hard time seeing how this is a significant issue, even when
> writing tests.
>
> It also seems to me that relaying the type of the constant as a suffix
> would help in cases that aren't so obvious.
>
> What am I missing?

I don't think it's terribly important, more like nuisance.  Tests
that check the value printed by the %E directive (I've been writing
lots of those lately -- see for example (*)) have to consistently
use this pattern:

     \[0-9\]+\[lu\]*

When the type of the %E argument is a type like size_t or similar
that can be an alias for unsigned long or unsigned int, it's easy
to make a mistake and hardcode either

     \[0-9\]+lu

or

     \[0-9\]+u

based on the target where the test is being developed and end
up with failures on targets where the actual type is the other.
Copying test cases exercising one type to those exercising the
other (say from int to long) is also more tedious than it would
be without the suffix.

Beyond tests, I have never found the suffix helpful in warnings
or errors, but I also haven't seen too many of them in released
versions of GCC.  With the work I've been doing on buffer
overflow where size expressions are routinely included in
the diagnostics, there are lots more of them.  In some (e.g.,
in all the -Wformat-length) I've taken care to avoid printing
the suffix by converting tree nodes to HOST_WIDE_INT.  But that's
cumbersome and error-prone, and leads to inconsistent output from
GCC for different diagnostics that don't do the same.

Martin

[*] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01672.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E (PR c/78165)
  2016-10-26 20:46 ` Joseph Myers
@ 2016-11-19 21:04   ` Martin Sebor
  2016-11-30  3:57     ` Jeff Law
  0 siblings, 1 reply; 8+ messages in thread
From: Martin Sebor @ 2016-11-19 21:04 UTC (permalink / raw)
  To: Joseph Myers, Jeff Law; +Cc: Gcc Patch List

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On 10/26/2016 02:46 PM, Joseph Myers wrote:
> On Wed, 26 Oct 2016, Martin Sebor wrote:
>
>> The attached patch implements one such approach by having the pretty
>> printer recognize the space format flag to suppress the type suffix,
>> so "%E" still prints the suffix but "% E" does not.  I did this to
>> preserve the existing output but I think it would be nicer to avoid
>> printing the suffix with %E and treat (for instance) the pound sign
>> as a request to add the suffix.  I have tested the attached patch
>> but not the alternative.
>
> I think printing the suffixes is a relic of %E being used to print full
> expressions.
>
> It's established by now that printing expressions reconstructed from trees
> is a bad idea; we can get better results by having precise location ranges
> and underlining the relevant part of the source.  So if we could make sure
> nowhere is trying the use %E (or %qE, etc.) with expressions that might
> not be constants, where the type might be relevant, then we'd have
> confidence that stopping printing the suffix is safe.  But given the low
> quality of the reconstructed expressions, it's probably safe anyway.
>
> (Most %qE uses are for identifiers not expressions.  If we give
> identifiers a different static type from "tree" - and certainly there
> isn't much reason for them to have the same type as expressions - then
> we'll need to change the format for either identifiers or expressions.)

Attached is a trivial patch to remove the suffix.  I didn't see
any failures in the test suite as a result.  I didn't attempt to
remove the type suffix from any tests (nor did my relatively
superficial search find any) but it will help simplify the tests
for my patches that are still in the review queue.

I should add to the rationale for the change I gave in my reply
to Jeff:

   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01692.html

that the print_dec[su] function that's sometimes used to format
integers in warning messages (e.g., by the -Walloca-larger-than
pass) doesn't add the suffix because it doesn't have knowledge
of the argument's type (it operates on wide_int).  That further
adds to the inconsistency in diagnostics.  This patch makes all
integers in diagnostics consistent regardless of their type.

Thanks
Martin

[-- Attachment #2: gcc-78165.diff --]
[-- Type: text/x-patch, Size: 1673 bytes --]

PR c/78165 - avoid printing type suffix for constants in %E output

gcc/c-family/ChangeLog:

	PR c/78165
	* c-pretty-print (pp_c_integer_constant): Avoid formatting type
	suffix.

diff --git a/gcc/c-family/c-pretty-print.c b/gcc/c-family/c-pretty-print.c
index 7ad5900..c32d0a0 100644
--- a/gcc/c-family/c-pretty-print.c
+++ b/gcc/c-family/c-pretty-print.c
@@ -904,15 +904,6 @@ pp_c_void_constant (c_pretty_printer *pp)
 static void
 pp_c_integer_constant (c_pretty_printer *pp, tree i)
 {
-  int idx;
-
-  /* We are going to compare the type of I to other types using
-     pointer comparison so we need to use its canonical type.  */
-  tree type =
-    TYPE_CANONICAL (TREE_TYPE (i))
-    ? TYPE_CANONICAL (TREE_TYPE (i))
-    : TREE_TYPE (i);
-
   if (tree_fits_shwi_p (i))
     pp_wide_integer (pp, tree_to_shwi (i));
   else if (tree_fits_uhwi_p (i))
@@ -929,24 +920,6 @@ pp_c_integer_constant (c_pretty_printer *pp, tree i)
       print_hex (wi, pp_buffer (pp)->digit_buffer);
       pp_string (pp, pp_buffer (pp)->digit_buffer);
     }
-  if (TYPE_UNSIGNED (type))
-    pp_character (pp, 'u');
-  if (type == long_integer_type_node || type == long_unsigned_type_node)
-    pp_character (pp, 'l');
-  else if (type == long_long_integer_type_node
-	   || type == long_long_unsigned_type_node)
-    pp_string (pp, "ll");
-  else for (idx = 0; idx < NUM_INT_N_ENTS; idx ++)
-    if (int_n_enabled_p[idx])
-      {
-	char buf[2+20];
-	if (type == int_n_trees[idx].signed_type
-	    || type == int_n_trees[idx].unsigned_type)
-	  {
-	    sprintf (buf, "I%d", int_n_data[idx].bitsize);
-	    pp_string (pp, buf);
-	  }
-      }
 }
 
 /* Print out a CHARACTER literal.  */

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E (PR c/78165)
  2016-11-19 21:04   ` [RFC PATCH] avoid printing type suffix with %E (PR c/78165) Martin Sebor
@ 2016-11-30  3:57     ` Jeff Law
  2016-12-09 10:17       ` Rainer Orth
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff Law @ 2016-11-30  3:57 UTC (permalink / raw)
  To: Martin Sebor, Joseph Myers; +Cc: Gcc Patch List

On 11/19/2016 02:04 PM, Martin Sebor wrote:
> On 10/26/2016 02:46 PM, Joseph Myers wrote:
>> On Wed, 26 Oct 2016, Martin Sebor wrote:
>>
>>> The attached patch implements one such approach by having the pretty
>>> printer recognize the space format flag to suppress the type suffix,
>>> so "%E" still prints the suffix but "% E" does not.  I did this to
>>> preserve the existing output but I think it would be nicer to avoid
>>> printing the suffix with %E and treat (for instance) the pound sign
>>> as a request to add the suffix.  I have tested the attached patch
>>> but not the alternative.
>>
>> I think printing the suffixes is a relic of %E being used to print full
>> expressions.
>>
>> It's established by now that printing expressions reconstructed from
>> trees
>> is a bad idea; we can get better results by having precise location
>> ranges
>> and underlining the relevant part of the source.  So if we could make
>> sure
>> nowhere is trying the use %E (or %qE, etc.) with expressions that might
>> not be constants, where the type might be relevant, then we'd have
>> confidence that stopping printing the suffix is safe.  But given the low
>> quality of the reconstructed expressions, it's probably safe anyway.
>>
>> (Most %qE uses are for identifiers not expressions.  If we give
>> identifiers a different static type from "tree" - and certainly there
>> isn't much reason for them to have the same type as expressions - then
>> we'll need to change the format for either identifiers or expressions.)
>
> Attached is a trivial patch to remove the suffix.  I didn't see
> any failures in the test suite as a result.  I didn't attempt to
> remove the type suffix from any tests (nor did my relatively
> superficial search find any) but it will help simplify the tests
> for my patches that are still in the review queue.
>
> I should add to the rationale for the change I gave in my reply
> to Jeff:
>
>   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01692.html
>
> that the print_dec[su] function that's sometimes used to format
> integers in warning messages (e.g., by the -Walloca-larger-than
> pass) doesn't add the suffix because it doesn't have knowledge
> of the argument's type (it operates on wide_int).  That further
> adds to the inconsistency in diagnostics.  This patch makes all
> integers in diagnostics consistent regardless of their type.
>
> Thanks
> Martin
>
> gcc-78165.diff
>
>
> PR c/78165 - avoid printing type suffix for constants in %E output
>
> gcc/c-family/ChangeLog:
>
> 	PR c/78165
> 	* c-pretty-print (pp_c_integer_constant): Avoid formatting type
> 	suffix.
I think you and Joseph have made a practical case for dropping the type 
suffix.

Ok for the trunk.

jeff

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E (PR c/78165)
  2016-11-30  3:57     ` Jeff Law
@ 2016-12-09 10:17       ` Rainer Orth
  2016-12-09 14:48         ` Martin Sebor
  0 siblings, 1 reply; 8+ messages in thread
From: Rainer Orth @ 2016-12-09 10:17 UTC (permalink / raw)
  To: Jeff Law; +Cc: Martin Sebor, Joseph Myers, Gcc Patch List

[-- Attachment #1: Type: text/plain, Size: 3784 bytes --]

Jeff Law <law@redhat.com> writes:

> On 11/19/2016 02:04 PM, Martin Sebor wrote:
>> On 10/26/2016 02:46 PM, Joseph Myers wrote:
>>> On Wed, 26 Oct 2016, Martin Sebor wrote:
>>>
>>>> The attached patch implements one such approach by having the pretty
>>>> printer recognize the space format flag to suppress the type suffix,
>>>> so "%E" still prints the suffix but "% E" does not.  I did this to
>>>> preserve the existing output but I think it would be nicer to avoid
>>>> printing the suffix with %E and treat (for instance) the pound sign
>>>> as a request to add the suffix.  I have tested the attached patch
>>>> but not the alternative.
>>>
>>> I think printing the suffixes is a relic of %E being used to print full
>>> expressions.
>>>
>>> It's established by now that printing expressions reconstructed from
>>> trees
>>> is a bad idea; we can get better results by having precise location
>>> ranges
>>> and underlining the relevant part of the source.  So if we could make
>>> sure
>>> nowhere is trying the use %E (or %qE, etc.) with expressions that might
>>> not be constants, where the type might be relevant, then we'd have
>>> confidence that stopping printing the suffix is safe.  But given the low
>>> quality of the reconstructed expressions, it's probably safe anyway.
>>>
>>> (Most %qE uses are for identifiers not expressions.  If we give
>>> identifiers a different static type from "tree" - and certainly there
>>> isn't much reason for them to have the same type as expressions - then
>>> we'll need to change the format for either identifiers or expressions.)
>>
>> Attached is a trivial patch to remove the suffix.  I didn't see
>> any failures in the test suite as a result.  I didn't attempt to
>> remove the type suffix from any tests (nor did my relatively
>> superficial search find any) but it will help simplify the tests
>> for my patches that are still in the review queue.
>>
>> I should add to the rationale for the change I gave in my reply
>> to Jeff:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01692.html
>>
>> that the print_dec[su] function that's sometimes used to format
>> integers in warning messages (e.g., by the -Walloca-larger-than
>> pass) doesn't add the suffix because it doesn't have knowledge
>> of the argument's type (it operates on wide_int).  That further
>> adds to the inconsistency in diagnostics.  This patch makes all
>> integers in diagnostics consistent regardless of their type.
>>
>> Thanks
>> Martin
>>
>> gcc-78165.diff
>>
>>
>> PR c/78165 - avoid printing type suffix for constants in %E output
>>
>> gcc/c-family/ChangeLog:
>>
>> 	PR c/78165
>> 	* c-pretty-print (pp_c_integer_constant): Avoid formatting type
>> 	suffix.
> I think you and Joseph have made a practical case for dropping the type
> suffix.
>
> Ok for the trunk.

The check-in lacked the gcc/testsuite ChangeLog.  Besides, the patch
caused a testsuite regression on Solaris with /bin/as (sparc and x86, 32
and 64-bit):

+FAIL: g++.dg/debug/dwarf2/typedef1.C  -std=gnu++11  scan-assembler-times DW_AT_name: "foo<1>"|"foo<1u>.."[^\\n]*DW_AT_name 1
+FAIL: g++.dg/debug/dwarf2/typedef1.C  -std=gnu++14  scan-assembler-times DW_AT_name: "foo<1>"|"foo<1u>.."[^\\n]*DW_AT_name 1
+FAIL: g++.dg/debug/dwarf2/typedef1.C  -std=gnu++98  scan-assembler-times DW_AT_name: "foo<1>"|"foo<1u>.."[^\\n]*DW_AT_name 1

Turns out an incomplete adjustment to the pattern, fixed as follows.
Will commit as obvious once testing on more configurations (gas, Linux)
has completed.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2016-12-09  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* g++.dg/debug/dwarf2/typedef1.C: Adjust pattern for last change.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: testsuite-dwarf2-typedef1.patch --]
[-- Type: text/x-patch, Size: 979 bytes --]

# HG changeset patch
# Parent  9cf8fdbb2f5fbebf7cf273c95a1d1e72567aa76c
Fix g++.dg/debug/dwarf2/typedef1.C

diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/typedef1.C b/gcc/testsuite/g++.dg/debug/dwarf2/typedef1.C
--- a/gcc/testsuite/g++.dg/debug/dwarf2/typedef1.C
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/typedef1.C
@@ -3,7 +3,7 @@
 // { dg-options "-gdwarf-2 -dA -fno-debug-types-section" }
 // { dg-do compile }
 // { dg-final { scan-assembler-times "DW_TAG_structure_type" 2 } }
-// { dg-final { scan-assembler-times "DW_AT_name: \"foo<1>\"|\"foo<1u>..\"\[^\n\]*DW_AT_name" 1 } }
+// { dg-final { scan-assembler-times "DW_AT_name: \"foo<1>\"|\"foo<1>..\"\[^\n\]*DW_AT_name" 1 } }
 // { dg-final { scan-assembler-times "DW_TAG_enumeration_type" 2 } }
 // { dg-final { scan-assembler-times "DW_AT_name: \"typedef foo<1>::type type\"|\"typedef foo<1>::type type..\"\[^\n\]*DW_AT_name" 1 } }
 // { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\) DW_TAG_enumeration_type" 1 } }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] avoid printing type suffix with %E (PR c/78165)
  2016-12-09 10:17       ` Rainer Orth
@ 2016-12-09 14:48         ` Martin Sebor
  0 siblings, 0 replies; 8+ messages in thread
From: Martin Sebor @ 2016-12-09 14:48 UTC (permalink / raw)
  To: Rainer Orth, Jeff Law; +Cc: Joseph Myers, Gcc Patch List

> The check-in lacked the gcc/testsuite ChangeLog.  Besides, the patch
> caused a testsuite regression on Solaris with /bin/as (sparc and x86, 32
> and 64-bit):
>
> +FAIL: g++.dg/debug/dwarf2/typedef1.C  -std=gnu++11  scan-assembler-times DW_AT_name: "foo<1>"|"foo<1u>.."[^\\n]*DW_AT_name 1
> +FAIL: g++.dg/debug/dwarf2/typedef1.C  -std=gnu++14  scan-assembler-times DW_AT_name: "foo<1>"|"foo<1u>.."[^\\n]*DW_AT_name 1
> +FAIL: g++.dg/debug/dwarf2/typedef1.C  -std=gnu++98  scan-assembler-times DW_AT_name: "foo<1>"|"foo<1u>.."[^\\n]*DW_AT_name 1
>
> Turns out an incomplete adjustment to the pattern, fixed as follows.
> Will commit as obvious once testing on more configurations (gas, Linux)
> has completed.

Thanks!  I tested x86_64 so it's possible that similar adjustments
will be needed in other target specific tests.  I'll be on the lookout
for more fallout.

Martin

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-12-09 14:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-26 16:37 [RFC PATCH] avoid printing type suffix with %E Martin Sebor
2016-10-26 20:46 ` Joseph Myers
2016-11-19 21:04   ` [RFC PATCH] avoid printing type suffix with %E (PR c/78165) Martin Sebor
2016-11-30  3:57     ` Jeff Law
2016-12-09 10:17       ` Rainer Orth
2016-12-09 14:48         ` Martin Sebor
2016-11-16 18:34 ` [RFC PATCH] avoid printing type suffix with %E Jeff Law
2016-11-16 18:54   ` Martin Sebor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).