[PATCH] Caret diagnostics

[PATCH] Caret diagnostics

[Manuel López-Ibáñez]

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

A simple implementation of caret diagnostics.

In the testsuite, pruning the caret output does not always work
because of several known deficiencies of DejaGNU, thus in some places
I disable the caret explicitly.

Bootstrapped and regression tested on x86_64-unknown-gnu-linux with
enable-languages=all,ada and -m32/-m64.

OK to commit?


2012-04-05  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR 24985
libstdc++-v3/
	* testsuite/lib/prune.exp: Handle caret.
libmudflap/
	* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
        * diagnostic.h (show_caret): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
        * input.c (read_line): New.
	(location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * testsuite/lib/prune.exp: Add -fno-diagnostics-show-caret.
        * testsuite/gcc.dg/torture/tls/tls.exp: Add -fno-diagnostics-show-caret.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
        * common.opt (fdiagnostics-show-caret): New option.

[-- Attachment #2: caret-diagnostics-20120406.diff --]
[-- Type: application/octet-stream, Size: 12577 bytes --]

Index: libstdc++-v3/testsuite/lib/prune.exp
===================================================================
--- libstdc++-v3/testsuite/lib/prune.exp	(revision 186103)
+++ libstdc++-v3/testsuite/lib/prune.exp	(working copy)
@@ -30,10 +30,18 @@ proc dg-prune-output { args } {
 }
 
 proc libstdc++-dg-prune { system text } {
     global additional_prunes
 
+#    send_user "Before:$text\n"
+
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+    # Also it may trim a source line by mistake since it matches repeated lines.
+    regsub -all "(^|\n) *\\^\n" $text "\n" text
+
     # Cygwin warns about -ffunction-sections
     regsub -all "(^|\n)\[^\n\]*: -ffunction-sections may affect debugging on some targets\[^\n\]*" $text "" text
 
     # Remove parts of warnings that refer to location of previous
     # definitions, etc as these confuse dejagnu
@@ -66,7 +74,8 @@ proc libstdc++-dg-prune { system text } 
 	    # Following regexp matches a complete line containing $p.
 	    regsub -all "(^|\n)\[^\n\]*$p\[^\n\]*" $text "" text
 	}
     }
 
+#    send_user "After:$text\n"
     return $text
 }
Index: libmudflap/testsuite/lib/libmudflap.exp
===================================================================
--- libmudflap/testsuite/lib/libmudflap.exp	(revision 186103)
+++ libmudflap/testsuite/lib/libmudflap.exp	(working copy)
@@ -296,10 +296,15 @@ proc libmudflap-dg-prune { system text }
     return $text
 }
 
 
 proc prune_gcc_output { text } {
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+    # Also it may trim a source line by mistake since it matches repeated lines.
+    regsub -all "(^|\n) *\\^\n" $text "\n" text
     regsub -all {(^|\n)[^\n]*ld: warning: libgcc_s[^\n]*not found[^\n]*try using[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function.*pthread_create[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*the use of .pthread.*is deprecated[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*Dwarf Error:.*FORM value: 14[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function[^\n]*} $text "" text
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 186103)
+++ gcc/diagnostic.c	(working copy)
@@ -98,10 +98,11 @@ diagnostic_initialize (diagnostic_contex
   context->warning_as_error_requested = false;
   context->n_opts = n_opts;
   context->classify_diagnostic = XNEWVEC (diagnostic_t, n_opts);
   for (i = 0; i < n_opts; i++)
     context->classify_diagnostic[i] = DK_UNSPECIFIED;
+  context->show_caret = false;
   context->show_option_requested = false;
   context->abort_on_error = false;
   context->show_column = false;
   context->pedantic_errors = false;
   context->permissive = false;
@@ -194,10 +195,67 @@ diagnostic_build_prefix (diagnostic_cont
      : context->show_column
      ? build_message_string ("%s:%d:%d: %s", s.file, s.line, s.column, text)
      : build_message_string ("%s:%d: %s", s.file, s.line, text));
 }
 
+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+static void
+diagnostic_show_locus (diagnostic_context * context,
+		       diagnostic_info *diagnostic)
+{
+  const char *line;
+  expanded_location s;
+  int k, real_width, real_column;
+  int max_width = 80, right_margin = 10;
+
+  if (!context->show_caret
+      || diagnostic->location <= BUILTINS_LOCATION)
+    return;
+
+  s = expand_location(diagnostic->location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+    return;
+
+  for (k = real_column = 0; k < s.column && line[k] != '\0'; k++)
+    real_column += (line[k] == '\t') ? 8 : 1;
+
+  for (real_width = real_column; line[k] != '\0'; k++)
+    real_width += (line[k] == '\t') ? 8 : 1;
+
+  /* If the line is longer than max_width and the column is too close
+     to max_width, skip part of the line until it is not so close.  */
+  right_margin = max_width - right_margin;
+  if (real_width >= max_width && real_column > right_margin) {
+    for (k = real_column - right_margin; k > 0; line++)
+      {
+	k -= (*line == '\t') ? 8 : 1;
+      }
+    real_column = right_margin + k;
+  }
+
+  fputc (' ', stderr);
+  while (max_width > 0 && *line != '\0')
+    {
+      if (*line != '\t')
+	{
+	  max_width--;
+	  fputc (*line, stderr);
+	} 
+      else 
+	{
+	  /* TABs are 8 spaces.  */
+	  max_width -= 8;
+	  fputs ("        ", stderr);
+	}
+      line++;
+    }
+  fputc ('\n', stderr);
+  fprintf (stderr, " %*c\n", real_column, '^');
+}
+
 /* Take any action which is expected to happen after the diagnostic
    is written out.  This function does not always return.  */
 static void
 diagnostic_action_after_output (diagnostic_context *context,
 				diagnostic_info *diagnostic)
@@ -547,10 +605,11 @@ diagnostic_report_diagnostic (diagnostic
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
   (*diagnostic_finalizer (context)) (context, diagnostic);
   pp_flush (context->printer);
+  diagnostic_show_locus (context, diagnostic);
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->x_data = NULL;
 
   context->lock--;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 186103)
+++ gcc/diagnostic.h	(working copy)
@@ -97,10 +97,14 @@ struct diagnostic_context
 
   /* For pragma push/pop.  */
   int *push_list;
   int n_push;
 
+  /* True if we should print the source line with a caret indicating
+     the location.  */
+  bool show_caret;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
Index: gcc/input.c
===================================================================
--- gcc/input.c	(revision 186103)
+++ gcc/input.c	(working copy)
@@ -48,10 +48,69 @@ expand_location (source_location loc)
     xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
 
   return xloc;
 }
 
+/* Reads one line from file into a static buffer.  */
+static const char *
+read_line (FILE *file)
+{
+  static char *string;
+  static size_t string_len;
+  size_t pos = 0;
+  char *ptr;
+
+  if (!string_len)
+    {
+      string_len = 200;
+      string = XNEWVEC (char, string_len);
+    }
+
+  while ((ptr = fgets (string + pos, string_len - pos, file)))
+    {
+      size_t len = strlen (string + pos);
+
+      if (string[pos + len - 1] == '\n')
+	{
+	  string[pos + len - 1] = 0;
+	  return string;
+	}
+      pos += len;
+      ptr = XNEWVEC (char, string_len * 2);
+      if (ptr)
+	{
+	  memcpy (ptr, string, pos);
+	  string = ptr;
+	  string_len += 2;
+	}
+      else
+	pos = 0;
+    }
+      
+  return pos ? string : NULL;
+}
+
+/* Return the physical source line that corresponds to xloc in a
+   buffer that is statically allocated.  The newline is replaced by
+   the null character.  */
+
+const char *
+location_get_source_line(expanded_location xloc)
+{
+  const char *buffer;
+  int lines = 1;
+  FILE *stream = xloc.file ? fopen (xloc.file, "r") : NULL;
+  if (!stream)
+    return NULL;
+
+  while ((buffer = read_line (stream)) && lines < xloc.line)
+    lines++;
+
+  fclose (stream);
+  return buffer;
+}
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
 /* Display a number as an integer multiple of either:
    - 1024, if said integer is >= to 10 K (in base 2)
Index: gcc/input.h
===================================================================
--- gcc/input.h	(revision 186103)
+++ gcc/input.h	(working copy)
@@ -36,10 +36,11 @@ extern GTY(()) struct line_maps *line_ta
    both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 extern char builtins_location_check[(BUILTINS_LOCATION
 				     < RESERVED_LOCATION_COUNT) ? 1 : -1];
 
 extern expanded_location expand_location (source_location);
+extern const char * location_get_source_line(expanded_location xloc);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
 typedef source_location location_t;
 
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c	(revision 186103)
+++ gcc/toplev.c	(working copy)
@@ -1167,10 +1167,12 @@ general_init (const char *argv0)
      finalizer -- for tokens resulting from macro macro expansion.  */
   diagnostic_finalizer (global_dc) = virt_loc_aware_diagnostic_finalizer;
   /* Set a default printer.  Language specific initializations will
      override it later.  */
   pp_format_decoder (global_dc->printer) = &default_tree_printer;
+  global_dc->show_caret
+    = global_options_init.x_flag_diagnostics_show_caret;
   global_dc->show_option_requested
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
   global_dc->internal_error = plugins_internal_error_function;
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp	(revision 186103)
+++ gcc/testsuite/lib/prune.exp	(working copy)
@@ -15,10 +15,12 @@
 # along with GCC; see the file COPYING3.  If not see
 # <http://www.gnu.org/licenses/>.
 
 # Prune messages from gcc that aren't useful.
 
+set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS"
+
 proc prune_gcc_output { text } {
     #send_user "Before:$text\n"
 
     regsub -all "(^|\n)(\[^\n\]*: )?In ((static member |lambda )?function|member|method|(copy )?constructor|destructor|instantiation|substitution|program|subroutine|block-data)\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[^\n\]*(: )?At (top level|global scope):\[^\n\]*" $text "" text
Index: gcc/testsuite/gcc.dg/torture/tls/tls.exp
===================================================================
--- gcc/testsuite/gcc.dg/torture/tls/tls.exp	(revision 186103)
+++ gcc/testsuite/gcc.dg/torture/tls/tls.exp	(working copy)
@@ -48,10 +48,10 @@ dg-init
 torture-init
 set-torture-options $TLS_TORTURE_OPTIONS {{}} $LTO_TORTURE_OPTIONS
 
 # Main loop.
 gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
-        $DEFAULT_CFLAGS
+        "-fno-diagnostics-show-caret $DEFAULT_CFLAGS"
 
 # All done.
 torture-finish
 dg-finish
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 186103)
+++ gcc/dwarf2out.c	(working copy)
@@ -18367,10 +18367,11 @@ gen_producer_string (void)
       case OPT_grecord_gcc_switches:
       case OPT_gno_record_gcc_switches:
       case OPT__output_pch_:
       case OPT_fdiagnostics_show_location_:
       case OPT_fdiagnostics_show_option:
+      case OPT_fdiagnostics_show_caret:
       case OPT_fverbose_asm:
       case OPT____:
       case OPT__sysroot_:
       case OPT_nostdinc:
       case OPT_nostdinc__:
Index: gcc/opts.c
===================================================================
--- gcc/opts.c	(revision 186103)
+++ gcc/opts.c	(working copy)
@@ -1497,10 +1497,14 @@ common_handle_option (struct gcc_options
       break;
 
     case OPT_fdiagnostics_show_location_:
       diagnostic_prefixing_rule (dc) = (diagnostic_prefixing_rule_t) value;
       break;
+ 
+    case OPT_fdiagnostics_show_caret:
+      dc->show_caret = value;
+      break;
 
     case OPT_fdiagnostics_show_option:
       dc->show_option_requested = value;
       break;
 
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 186103)
+++ gcc/common.opt	(working copy)
@@ -997,10 +997,14 @@ EnumValue
 Enum(diagnostic_prefixing_rule) String(once) Value(DIAGNOSTICS_SHOW_PREFIX_ONCE)
 
 EnumValue
 Enum(diagnostic_prefixing_rule) String(every-line) Value(DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE)
 
+fdiagnostics-show-caret
+Common Var(flag_diagnostics_show_caret) Init(1)
+Show the source line with a caret indicating the column
+
 fdiagnostics-show-option
 Common Var(flag_diagnostics_show_option) Init(1)
 Amend appropriate diagnostic messages with the command line option that controls them
 
 fdisable-

Re: [PATCH] Caret diagnostics

[Mike Stump]

On Apr 6, 2012, at 1:11 AM, Manuel López-Ibáñez wrote:
> A simple implementation of caret diagnostics.
> 
> In the testsuite, pruning the caret output does not always work
> because of several known deficiencies of DejaGNU, thus in some places
> I disable the caret explicitly.
> 
> Bootstrapped and regression tested on x86_64-unknown-gnu-linux with
> enable-languages=all,ada and -m32/-m64.
> 
> OK to commit?

Could someone take Objective-C++ for a spin?  It builds and runs pretty quickly, non-bootstrap is fine.

The testsuite changes are fine, though, it feels like you're missing the main prune routine?

Nice work, love to see it go in.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 6 April 2012 15:42, Mike Stump <mikestump@comcast.net> wrote:
> On Apr 6, 2012, at 1:11 AM, Manuel López-Ibáñez wrote:
>> A simple implementation of caret diagnostics.
>>
>> In the testsuite, pruning the caret output does not always work
>> because of several known deficiencies of DejaGNU, thus in some places
>> I disable the caret explicitly.
>>
>> Bootstrapped and regression tested on x86_64-unknown-gnu-linux with
>> enable-languages=all,ada and -m32/-m64.
>>
>> OK to commit?
>
> Could someone take Objective-C++ for a spin?  It builds and runs pretty quickly, non-bootstrap is fine.
>
> The testsuite changes are fine, though, it feels like you're missing the main prune routine?

I didn't change the prune routine because it doesn't work reliably
(because of DejaGNU problems PR12096 and PR30612 and others mentioned
in PR24985). That is why I set -fno-diagnostics-show-caret there.
Other places that have their own prune routines didn't show any
problems, so I prune the output instead of adding
-fno-diagnostics-show-caret to every command.

> Nice work, love to see it go in.

Thanks,

Manuel.

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/06/2012 04:11 AM, Manuel López-Ibáñez wrote:
> +++ gcc/testsuite/gcc.dg/torture/tls/tls.exp	(working copy)
> @@ -48,10 +48,10 @@ dg-init
>  torture-init
>  set-torture-options $TLS_TORTURE_OPTIONS {{}} $LTO_TORTURE_OPTIONS
>
>  # Main loop.
>  gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
> -        $DEFAULT_CFLAGS
> +        "-fno-diagnostics-show-caret $DEFAULT_CFLAGS"

I don't think this is needed since you're already adding it to 
TEST_ALWAYS_FLAGS.  The tests pass without this for me.

> +  int max_width = 80, right_margin = 10;

Let's not hardcode these numbers.  We should use getenv("COLUMNS") for 
the width if it's set; otherwise I would lean toward unlimited width. 
And I'm not sure why we need a right margin at all.

> +  s = expand_location(diagnostic->location);
> +  line = location_get_source_line (s);

Why not expand the location inside location_get_source_line?

Jason

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 7 April 2012 00:04, Jason Merrill <jason@redhat.com> wrote:
> On 04/06/2012 04:11 AM, Manuel López-Ibáńez wrote:
>>
>> +++ gcc/testsuite/gcc.dg/torture/tls/tls.exp    (working copy)
>> @@ -48,10 +48,10 @@ dg-init
>>  torture-init
>>  set-torture-options $TLS_TORTURE_OPTIONS {{}} $LTO_TORTURE_OPTIONS
>>
>>  # Main loop.
>>  gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
>> -        $DEFAULT_CFLAGS
>> +        "-fno-diagnostics-show-caret $DEFAULT_CFLAGS"
>
>
> I don't think this is needed since you're already adding it to
> TEST_ALWAYS_FLAGS.  The tests pass without this for me.
>

Good. I will try without, but the output does not show the flag two
times, which make me think that tls.exp is not using
TEST_ALWAYS_FLAGS. But maybe I was looking at the wrong place.

>> +  int max_width = 80, right_margin = 10;
>
>
> Let's not hardcode these numbers.  We should use getenv("COLUMNS") for the

Fine, I'll try that.

> width if it's set; otherwise I would lean toward unlimited width. And I'm
> not sure why we need a right margin at all.

The right margin is because:

* We don't want to do wrapping. Otherwise the caret looks strange. So
either we cut long lines or we leave the maximum width unlimited.
* If we cut long lines, and the line is too long and the caret is too
close to the right margin, it is better to cut the line at the
beginning to show the part pointed by the caret within the maximum
width, with some margin to the right. That is, we don't want to show:

 very_very_long_line with something w
                                                     ^
We want to show:

 long_line with something wrong on it
                                      ^


>> +  s = expand_location(diagnostic->location);
>> +  line = location_get_source_line (s);
>
>
> Why not expand the location inside location_get_source_line?

Well, we have to expand outside anyway, because I need the column
info, so it seemed a waste to expand twice. Do you have a strong
preference for expanding twice? I don't care so much either way.

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/06/2012 06:30 PM, Manuel López-Ibáñez wrote:
>> width if it's set; otherwise I would lean toward unlimited width. And I'm
>> not sure why we need a right margin at all.
>
> The right margin is because:
 >[snip]

Ah, I read "margin" and assumed it meant you were leaving blank space at 
the right side of the screen.  Now I understand.

> Well, we have to expand outside anyway, because I need the column
> info, so it seemed a waste to expand twice. Do you have a strong
> preference for expanding twice? I don't care so much either way.

No, I guess it's fine this way.

Jason

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

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

On 7 April 2012 04:31, Jason Merrill <jason@redhat.com> wrote:
> On 04/06/2012 06:30 PM, Manuel López-Ibáñez wrote:
>>>
>>> width if it's set; otherwise I would lean toward unlimited width. And I'm
>>> not sure why we need a right margin at all.
>>
>>
>> The right margin is because:
>
>>[snip]
>
> Ah, I read "margin" and assumed it meant you were leaving blank space at the
> right side of the screen.  Now I understand.

I'll be happy to change it to whatever is more understandable. I think
in CSS is called "padding".

New version attached. Bootstrapped and regression tested including obj-c++.

Cheers,

Manuel.

2012-04-05  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR 24985
libstdc++-v3/
	* testsuite/lib/prune.exp: Handle caret.
libmudflap/
	* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
        * diagnostic.h (show_caret): Declare.
	(diagnostic_show_locus): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
	(getenv_columns): New function.
        * input.c (read_line): New.
	(location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
        * common.opt (fdiagnostics-show-caret): New option.
        * testsuite/lib/prune.exp: Add -fno-diagnostics-show-caret.

[-- Attachment #2: caret-diagnostics-20120408.diff --]
[-- Type: application/octet-stream, Size: 12973 bytes --]

Index: libstdc++-v3/testsuite/lib/prune.exp
===================================================================
--- libstdc++-v3/testsuite/lib/prune.exp	(revision 186103)
+++ libstdc++-v3/testsuite/lib/prune.exp	(working copy)
@@ -30,10 +30,18 @@ proc dg-prune-output { args } {
 }
 
 proc libstdc++-dg-prune { system text } {
     global additional_prunes
 
+#    send_user "Before:$text\n"
+
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+    # Also it may trim a source line by mistake since it matches repeated lines.
+    regsub -all "(^|\n) *\\^\n" $text "\n" text
+
     # Cygwin warns about -ffunction-sections
     regsub -all "(^|\n)\[^\n\]*: -ffunction-sections may affect debugging on some targets\[^\n\]*" $text "" text
 
     # Remove parts of warnings that refer to location of previous
     # definitions, etc as these confuse dejagnu
@@ -66,7 +74,8 @@ proc libstdc++-dg-prune { system text } 
 	    # Following regexp matches a complete line containing $p.
 	    regsub -all "(^|\n)\[^\n\]*$p\[^\n\]*" $text "" text
 	}
     }
 
+#    send_user "After:$text\n"
     return $text
 }
Index: libmudflap/testsuite/lib/libmudflap.exp
===================================================================
--- libmudflap/testsuite/lib/libmudflap.exp	(revision 186103)
+++ libmudflap/testsuite/lib/libmudflap.exp	(working copy)
@@ -296,10 +296,15 @@ proc libmudflap-dg-prune { system text }
     return $text
 }
 
 
 proc prune_gcc_output { text } {
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+    # Also it may trim a source line by mistake since it matches repeated lines.
+    regsub -all "(^|\n) *\\^\n" $text "\n" text
     regsub -all {(^|\n)[^\n]*ld: warning: libgcc_s[^\n]*not found[^\n]*try using[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function.*pthread_create[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*the use of .pthread.*is deprecated[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*Dwarf Error:.*FORM value: 14[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function[^\n]*} $text "" text
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 186103)
+++ gcc/diagnostic.c	(working copy)
@@ -98,10 +98,11 @@ diagnostic_initialize (diagnostic_contex
   context->warning_as_error_requested = false;
   context->n_opts = n_opts;
   context->classify_diagnostic = XNEWVEC (diagnostic_t, n_opts);
   for (i = 0; i < n_opts; i++)
     context->classify_diagnostic[i] = DK_UNSPECIFIED;
+  context->show_caret = false;
   context->show_option_requested = false;
   context->abort_on_error = false;
   context->show_column = false;
   context->pedantic_errors = false;
   context->permissive = false;
@@ -194,10 +195,83 @@ diagnostic_build_prefix (diagnostic_cont
      : context->show_column
      ? build_message_string ("%s:%d:%d: %s", s.file, s.line, s.column, text)
      : build_message_string ("%s:%d: %s", s.file, s.line, text));
 }
 
+/* Return the value of the getenv("COLUMNS") as an integer. If the
+   value is not set or it is not a positive integer, then return
+   INT_MAX.  */
+static int
+getenv_columns (void)
+{
+  const char * s = getenv ("COLUMNS");
+  if (s != NULL) {
+    int n = atoi (s);
+    if (n > 0)
+      return n;
+  }
+  return INT_MAX;
+}
+
+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+void
+diagnostic_show_locus (diagnostic_context * context,
+		       const diagnostic_info *diagnostic)
+{
+  const char *line;
+  expanded_location s;
+  int k, real_width, real_column;
+  int max_width, right_margin = 10;
+
+  if (!context->show_caret
+      || diagnostic->location <= BUILTINS_LOCATION)
+    return;
+
+  s = expand_location(diagnostic->location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+    return;
+
+  max_width = getenv_columns ();
+  for (k = real_column = 0; k < s.column && line[k] != '\0'; k++)
+    real_column += (line[k] == '\t') ? 8 : 1;
+
+  for (real_width = real_column; line[k] != '\0'; k++)
+    real_width += (line[k] == '\t') ? 8 : 1;
+
+  /* If the line is longer than max_width and the column is too close
+     to max_width, skip part of the line until it is not so close.  */
+  right_margin = max_width - right_margin;
+  if (real_width >= max_width && real_column > right_margin) {
+    for (k = real_column - right_margin; k > 0; line++)
+      {
+	k -= (*line == '\t') ? 8 : 1;
+      }
+    real_column = right_margin + k;
+  }
+
+  fputc (' ', stderr);
+  while (max_width > 0 && *line != '\0')
+    {
+      if (*line != '\t')
+	{
+	  max_width--;
+	  fputc (*line, stderr);
+	} 
+      else 
+	{
+	  /* TABs are 8 spaces.  */
+	  max_width -= 8;
+	  fputs ("        ", stderr);
+	}
+      line++;
+    }
+  fputc ('\n', stderr);
+  fprintf (stderr, " %*c\n", real_column, '^');
+}
+
 /* Take any action which is expected to happen after the diagnostic
    is written out.  This function does not always return.  */
 static void
 diagnostic_action_after_output (diagnostic_context *context,
 				diagnostic_info *diagnostic)
@@ -547,10 +621,11 @@ diagnostic_report_diagnostic (diagnostic
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
   (*diagnostic_finalizer (context)) (context, diagnostic);
   pp_flush (context->printer);
+  diagnostic_show_locus (context, diagnostic);
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->x_data = NULL;
 
   context->lock--;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 186103)
+++ gcc/diagnostic.h	(working copy)
@@ -97,10 +97,14 @@ struct diagnostic_context
 
   /* For pragma push/pop.  */
   int *push_list;
   int n_push;
 
+  /* True if we should print the source line with a caret indicating
+     the location.  */
+  bool show_caret;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
@@ -252,10 +256,11 @@ extern diagnostic_context *global_dc;
 
 /* Diagnostic related functions.  */
 extern void diagnostic_initialize (diagnostic_context *, int);
 extern void diagnostic_finish (diagnostic_context *);
 extern void diagnostic_report_current_module (diagnostic_context *, location_t);
+extern void diagnostic_show_locus (diagnostic_context *, const diagnostic_info *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */,
Index: gcc/input.c
===================================================================
--- gcc/input.c	(revision 186103)
+++ gcc/input.c	(working copy)
@@ -48,10 +48,69 @@ expand_location (source_location loc)
     xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
 
   return xloc;
 }
 
+/* Reads one line from file into a static buffer.  */
+static const char *
+read_line (FILE *file)
+{
+  static char *string;
+  static size_t string_len;
+  size_t pos = 0;
+  char *ptr;
+
+  if (!string_len)
+    {
+      string_len = 200;
+      string = XNEWVEC (char, string_len);
+    }
+
+  while ((ptr = fgets (string + pos, string_len - pos, file)))
+    {
+      size_t len = strlen (string + pos);
+
+      if (string[pos + len - 1] == '\n')
+	{
+	  string[pos + len - 1] = 0;
+	  return string;
+	}
+      pos += len;
+      ptr = XNEWVEC (char, string_len * 2);
+      if (ptr)
+	{
+	  memcpy (ptr, string, pos);
+	  string = ptr;
+	  string_len += 2;
+	}
+      else
+	pos = 0;
+    }
+      
+  return pos ? string : NULL;
+}
+
+/* Return the physical source line that corresponds to xloc in a
+   buffer that is statically allocated.  The newline is replaced by
+   the null character.  */
+
+const char *
+location_get_source_line(expanded_location xloc)
+{
+  const char *buffer;
+  int lines = 1;
+  FILE *stream = xloc.file ? fopen (xloc.file, "r") : NULL;
+  if (!stream)
+    return NULL;
+
+  while ((buffer = read_line (stream)) && lines < xloc.line)
+    lines++;
+
+  fclose (stream);
+  return buffer;
+}
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
 /* Display a number as an integer multiple of either:
    - 1024, if said integer is >= to 10 K (in base 2)
Index: gcc/input.h
===================================================================
--- gcc/input.h	(revision 186103)
+++ gcc/input.h	(working copy)
@@ -36,10 +36,11 @@ extern GTY(()) struct line_maps *line_ta
    both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 extern char builtins_location_check[(BUILTINS_LOCATION
 				     < RESERVED_LOCATION_COUNT) ? 1 : -1];
 
 extern expanded_location expand_location (source_location);
+extern const char * location_get_source_line(expanded_location xloc);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
 typedef source_location location_t;
 
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c	(revision 186103)
+++ gcc/toplev.c	(working copy)
@@ -1167,10 +1167,12 @@ general_init (const char *argv0)
      finalizer -- for tokens resulting from macro macro expansion.  */
   diagnostic_finalizer (global_dc) = virt_loc_aware_diagnostic_finalizer;
   /* Set a default printer.  Language specific initializations will
      override it later.  */
   pp_format_decoder (global_dc->printer) = &default_tree_printer;
+  global_dc->show_caret
+    = global_options_init.x_flag_diagnostics_show_caret;
   global_dc->show_option_requested
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
   global_dc->internal_error = plugins_internal_error_function;
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp	(revision 186103)
+++ gcc/testsuite/lib/prune.exp	(working copy)
@@ -15,10 +15,12 @@
 # along with GCC; see the file COPYING3.  If not see
 # <http://www.gnu.org/licenses/>.
 
 # Prune messages from gcc that aren't useful.
 
+set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS"
+
 proc prune_gcc_output { text } {
     #send_user "Before:$text\n"
 
     regsub -all "(^|\n)(\[^\n\]*: )?In ((static member |lambda )?function|member|method|(copy )?constructor|destructor|instantiation|substitution|program|subroutine|block-data)\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[^\n\]*(: )?At (top level|global scope):\[^\n\]*" $text "" text
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 186103)
+++ gcc/dwarf2out.c	(working copy)
@@ -18367,10 +18367,11 @@ gen_producer_string (void)
       case OPT_grecord_gcc_switches:
       case OPT_gno_record_gcc_switches:
       case OPT__output_pch_:
       case OPT_fdiagnostics_show_location_:
       case OPT_fdiagnostics_show_option:
+      case OPT_fdiagnostics_show_caret:
       case OPT_fverbose_asm:
       case OPT____:
       case OPT__sysroot_:
       case OPT_nostdinc:
       case OPT_nostdinc__:
Index: gcc/opts.c
===================================================================
--- gcc/opts.c	(revision 186103)
+++ gcc/opts.c	(working copy)
@@ -1497,10 +1497,14 @@ common_handle_option (struct gcc_options
       break;
 
     case OPT_fdiagnostics_show_location_:
       diagnostic_prefixing_rule (dc) = (diagnostic_prefixing_rule_t) value;
       break;
+ 
+    case OPT_fdiagnostics_show_caret:
+      dc->show_caret = value;
+      break;
 
     case OPT_fdiagnostics_show_option:
       dc->show_option_requested = value;
       break;
 
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 186103)
+++ gcc/common.opt	(working copy)
@@ -997,10 +997,14 @@ EnumValue
 Enum(diagnostic_prefixing_rule) String(once) Value(DIAGNOSTICS_SHOW_PREFIX_ONCE)
 
 EnumValue
 Enum(diagnostic_prefixing_rule) String(every-line) Value(DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE)
 
+fdiagnostics-show-caret
+Common Var(flag_diagnostics_show_caret) Init(1)
+Show the source line with a caret indicating the column
+
 fdiagnostics-show-option
 Common Var(flag_diagnostics_show_option) Init(1)
 Amend appropriate diagnostic messages with the command line option that controls them
 
 fdisable-

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/07/2012 06:29 PM, Manuel López-Ibáñez wrote:
> I'll be happy to change it to whatever is more understandable. I think
> in CSS is called "padding".

That wouldn't be any clearer; my confusion was that I thought you were 
padding the right side of the source line, but you aren't; you are only 
making sure that the caret isn't too close to the edge of the terminal. 
  No need to change anything here.

> +getenv_columns (void)

I had been thinking to check COLUMNS once at the beginning of 
compilation; I don't think the value can change while the compiler is 
running since we don't respond to SIGWINCH.  And let's use this value in 
c_common_initialize_diagnostics, too.

> +    # Also it may trim a source line by mistake since it matches repeated lines.

Let's clarify this comment; because dejagnu gets the whole compiler 
output as a single string, .* in a test regexp can match across multiple 
lines, which with caret diagnostics is likely to mean that the beginning 
of the regexp matches the error message and the end of the regexp 
matches the regexp itself in the printed source line.

Jason

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>
>> I'll be happy to change it to whatever is more understandable. I think
>> in CSS is called "padding".
>
>
> That wouldn't be any clearer; my confusion was that I thought you were
> padding the right side of the source line, but you aren't; you are only
> making sure that the caret isn't too close to the edge of the terminal.  No
> need to change anything here.
>
>> +getenv_columns (void)
>
>
> I had been thinking to check COLUMNS once at the beginning of compilation; I
> don't think the value can change while the compiler is running since we
> don't respond to SIGWINCH.  And let's use this value in
> c_common_initialize_diagnostics, too.

That code is useless. It is overridden by cxx_initialize_diagnostics
(line 2429). It is not strange that nobody noticed this because the
pretty-printer code is hard to understand and impossible to debug. I'd
rather not touch it if I can avoid it.

I am not even sure if that is the correct way to set the line-cut-off
in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
knows what is the correct way?

Also, using COLUMNS to initialize pp's line-cut-off will change the
current default (which is unlimited despite what invoke.texi says).

I can add "locus_max_width" to diagnostic_context, and use
getenv_columns to initialize it once. Would that be ok?

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Sun, Apr 8, 2012 at 7:06 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>>
>>> I'll be happy to change it to whatever is more understandable. I think
>>> in CSS is called "padding".
>>
>>
>> That wouldn't be any clearer; my confusion was that I thought you were
>> padding the right side of the source line, but you aren't; you are only
>> making sure that the caret isn't too close to the edge of the terminal.  No
>> need to change anything here.
>>
>>> +getenv_columns (void)
>>
>>
>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>> don't think the value can change while the compiler is running since we
>> don't respond to SIGWINCH.  And let's use this value in
>> c_common_initialize_diagnostics, too.
>
> That code is useless.

I do not understand what you by that.  Could you elaborate?
c-family/ChangeLog says it was split out of c_common_init_options.

> It is overridden by cxx_initialize_diagnostics
> (line 2429). It is not strange that nobody noticed this because the
> pretty-printer code is hard to understand and impossible to debug.

I find the syllogism a bit specious.

>  I'd rather not touch it if I can avoid it.
>
> I am not even sure if that is the correct way to set the line-cut-off
> in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
> knows what is the correct way?

The documentation of pp_base_set_line_maximum_length says:

/* Sets the number of maximum characters per line PRETTY-PRINTER can
   output in line-wrapping mode.  A LENGTH value 0 suppresses
   line-wrapping.  */

>
> Also, using COLUMNS to initialize pp's line-cut-off will change the
> current default (which is unlimited despite what invoke.texi says).
>
> I can add "locus_max_width" to diagnostic_context, and use
> getenv_columns to initialize it once. Would that be ok?

I am not sure how its semantics would differ from pp_line_cutoff
but if you are going to disable line-wrapping mode (which this
effectively does), then the structure pp_wrapping_mode_t is
where to place this kind of information.

>
> Cheers,
>
> Manuel.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 8 April 2012 17:14, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
> On Sun, Apr 8, 2012 at 7:06 AM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
>> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>>> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>>>
>>>> I'll be happy to change it to whatever is more understandable. I think
>>>> in CSS is called "padding".
>>>
>>>
>>> That wouldn't be any clearer; my confusion was that I thought you were
>>> padding the right side of the source line, but you aren't; you are only
>>> making sure that the caret isn't too close to the edge of the terminal.  No
>>> need to change anything here.
>>>
>>>> +getenv_columns (void)
>>>
>>>
>>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>>> don't think the value can change while the compiler is running since we
>>> don't respond to SIGWINCH.  And let's use this value in
>>> c_common_initialize_diagnostics, too.
>>
>> That code is useless.
>
> I do not understand what you by that.  Could you elaborate?
> c-family/ChangeLog says it was split out of c_common_init_options.

I mean it has no effect because cxx_initialize_diagnostics overrides
it before returning. Step in that function and you will see
diagnostic_line_cutoff (context) change from 0 to 80 to 0.

>> It is overridden by cxx_initialize_diagnostics
>> (line 2429). It is not strange that nobody noticed this because the
>> pretty-printer code is hard to understand and impossible to debug.
>
> I find the syllogism a bit specious.

Sorry,  I didn't express myself in the best way out of frustration. I
mean that the pretty-printer code is quite complex since there are
many interacting parts, it emulates inheritance in plain C, and there
is no encapsulation, so variables are sometimes modified directly and
other times by using various functions. The abundant use of
preprocessor macros as accessors and for implementing inheritance
makes quite hard to debug the code in gdb. But this is a well-known
problem of the whole GCC codebase. Probably, there is no better way to
implement the same functionality in plain C, or perhaps gdb is not
powerful enough, or perhaps I am using it incorrectly. In any case, I
am having a very hard time following how the various pretty-printers
are initialized.

>>  I'd rather not touch it if I can avoid it.
>>
>> I am not even sure if that is the correct way to set the line-cut-off
>> in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
>> knows what is the correct way?
>
> The documentation of pp_base_set_line_maximum_length says:
>
> /* Sets the number of maximum characters per line PRETTY-PRINTER can
>   output in line-wrapping mode.  A LENGTH value 0 suppresses
>   line-wrapping.  */

So when is it appropriate to use

diagnostic_line_cutoff (context) = 80;

like in the above code?

>> Also, using COLUMNS to initialize pp's line-cut-off will change the
>> current default (which is unlimited despite what invoke.texi says).
>>
>> I can add "locus_max_width" to diagnostic_context, and use
>> getenv_columns to initialize it once. Would that be ok?
>
> I am not sure how its semantics would differ from pp_line_cutoff
> but if you are going to disable line-wrapping mode (which this
> effectively does), then the structure pp_wrapping_mode_t is
> where to place this kind of information.

Actually, what Jason is proposing is to initialize line_cutoff to
getevn("COLUMNS") if present or to unlimited if not, and then use that
for the caret diagnostic. I see two problems with this:

* We change the current default, which is no line-wrapping ever.

* Where should the default be set? By each FE? by diagnostic.c? by
pretty-printer.c?

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>
>> +getenv_columns (void)
>
>
> I had been thinking to check COLUMNS once at the beginning of compilation; I
> don't think the value can change while the compiler is running since we
> don't respond to SIGWINCH.  And let's use this value in
> c_common_initialize_diagnostics, too.
>

To focus on the issue at hand, and independently of whether any bugs
exist or not, and whether I am capable or not to fix them. What you
are proposing is to change the current default of no line-wrapping to
line-wrap at getenv("COLUMNS"), isn't it? And then, to use the same
value for the max_width in the function printing the caret, is that
correct?

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Sun, Apr 8, 2012 at 11:10 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 8 April 2012 17:14, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
>> On Sun, Apr 8, 2012 at 7:06 AM, Manuel López-Ibáñez
>> <lopezibanez@gmail.com> wrote:
>>> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>>>> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>>>>
>>>>> I'll be happy to change it to whatever is more understandable. I think
>>>>> in CSS is called "padding".
>>>>
>>>>
>>>> That wouldn't be any clearer; my confusion was that I thought you were
>>>> padding the right side of the source line, but you aren't; you are only
>>>> making sure that the caret isn't too close to the edge of the terminal.  No
>>>> need to change anything here.
>>>>
>>>>> +getenv_columns (void)
>>>>
>>>>
>>>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>>>> don't think the value can change while the compiler is running since we
>>>> don't respond to SIGWINCH.  And let's use this value in
>>>> c_common_initialize_diagnostics, too.
>>>
>>> That code is useless.
>>
>> I do not understand what you by that.  Could you elaborate?
>> c-family/ChangeLog says it was split out of c_common_init_options.
>
> I mean it has no effect because cxx_initialize_diagnostics overrides
> it before returning. Step in that function and you will see
> diagnostic_line_cutoff (context) change from 0 to 80 to 0.

OK.

>
>>> It is overridden by cxx_initialize_diagnostics
>>> (line 2429). It is not strange that nobody noticed this because the
>>> pretty-printer code is hard to understand and impossible to debug.
>>
>> I find the syllogism a bit specious.
>
> Sorry,  I didn't express myself in the best way out of frustration. I
> mean that the pretty-printer code is quite complex since there are
> many interacting parts, it emulates inheritance in plain C, and there
> is no encapsulation, so variables are sometimes modified directly and
> other times by using various functions. The abundant use of
> preprocessor macros as accessors and for implementing inheritance
> makes quite hard to debug the code in gdb. But this is a well-known
> problem of the whole GCC codebase. Probably, there is no better way to
> implement the same functionality in plain C, or perhaps gdb is not
> powerful enough, or perhaps I am using it incorrectly. In any case, I
> am having a very hard time following how the various pretty-printers
> are initialized.

Many of the pretty printer macros were introduced to abstract away
direct access to the fields.  The fact that some fields are accessed
directly in client codes is a bug in client codes, not a feature of the
pretty printer interface.  Alas C does not provide a convenient access
control, so these access violations go undetected when they slip
through careful reviews.  GDB has support for macros but it true
that debugging macro-based interfaces require some gymnastics,
and this one is no exception.

>
>>>  I'd rather not touch it if I can avoid it.
>>>
>>> I am not even sure if that is the correct way to set the line-cut-off
>>> in PP. The option fmessage-length uses pp_set_line_maximum_length. Who
>>> knows what is the correct way?
>>
>> The documentation of pp_base_set_line_maximum_length says:
>>
>> /* Sets the number of maximum characters per line PRETTY-PRINTER can
>>   output in line-wrapping mode.  A LENGTH value 0 suppresses
>>   line-wrapping.  */
>
> So when is it appropriate to use
>
> diagnostic_line_cutoff (context) = 80;
>
> like in the above code?

First let me say this:  if you are doing diagnostics with caret, then you do
not want line wrapping, otherwise it gets a bit ugly.  So you need
a way to tell the diagnostic outputter that you do not want line wrap.

diagnostic_line_cutoff suggests the maximum number of characters that
the diagnostic outputter could send before emitting a newline to begin
line wrap.  0 suggests no line wrap.

So, I would not try to reuse that field -- otherwise, it can get messed up.
You suggested in your previous mail to use a distinct field to store the
width the terminal; I think that is a better option.

>
>>> Also, using COLUMNS to initialize pp's line-cut-off will change the
>>> current default (which is unlimited despite what invoke.texi says).
>>>
>>> I can add "locus_max_width" to diagnostic_context, and use
>>> getenv_columns to initialize it once. Would that be ok?
>>
>> I am not sure how its semantics would differ from pp_line_cutoff
>> but if you are going to disable line-wrapping mode (which this
>> effectively does), then the structure pp_wrapping_mode_t is
>> where to place this kind of information.
>
> Actually, what Jason is proposing is to initialize line_cutoff to
> getevn("COLUMNS") if present or to unlimited if not, and then use that
> for the caret diagnostic. I see two problems with this:
>
> * We change the current default, which is no line-wrapping ever.
>
> * Where should the default be set? By each FE? by diagnostic.c? by
> pretty-printer.c?
>

I would suggest the front-ends that want it.  If all front ends want it
then it makes sense to set it once for all in diagnostic.c.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Sun, Apr 8, 2012 at 11:13 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>
>>> +getenv_columns (void)
>>
>>
>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>> don't think the value can change while the compiler is running since we
>> don't respond to SIGWINCH.  And let's use this value in
>> c_common_initialize_diagnostics, too.
>>
>
> To focus on the issue at hand, and independently of whether any bugs
> exist or not, and whether I am capable or not to fix them. What you
> are proposing is to change the current default of no line-wrapping to
> line-wrap at getenv("COLUMNS"), isn't it?

yes.

>  And then, to use the same
> value for the max_width in the function printing the caret, is that
> correct?

yes.

>
> Cheers,
>
> Manuel.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 8 April 2012 18:35, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
> On Sun, Apr 8, 2012 at 11:13 AM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
>> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>>> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>>
>>>> +getenv_columns (void)
>>>
>>>
>>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>>> don't think the value can change while the compiler is running since we
>>> don't respond to SIGWINCH.  And let's use this value in
>>> c_common_initialize_diagnostics, too.
>>>
>>
>> To focus on the issue at hand, and independently of whether any bugs
>> exist or not, and whether I am capable or not to fix them. What you
>> are proposing is to change the current default of no line-wrapping to
>> line-wrap at getenv("COLUMNS"), isn't it?
>
> yes.

OK, so to do this, I have to figure out where the pretty-printer is
initialized in each FE. For C++ there are two different places:

init_error() initializes cxx_pp
whereas cxx_initialize_diagnostics seems to initialize a different
pretty-printer for C++.

What is the difference between the two?

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Sun, Apr 8, 2012 at 11:52 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 8 April 2012 18:35, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
>> On Sun, Apr 8, 2012 at 11:13 AM, Manuel López-Ibáñez
>> <lopezibanez@gmail.com> wrote:
>>> On 8 April 2012 06:09, Jason Merrill <jason@redhat.com> wrote:
>>>> On 04/07/2012 06:29 PM, Manuel López-Ibáńez wrote:
>>>>
>>>>> +getenv_columns (void)
>>>>
>>>>
>>>> I had been thinking to check COLUMNS once at the beginning of compilation; I
>>>> don't think the value can change while the compiler is running since we
>>>> don't respond to SIGWINCH.  And let's use this value in
>>>> c_common_initialize_diagnostics, too.
>>>>
>>>
>>> To focus on the issue at hand, and independently of whether any bugs
>>> exist or not, and whether I am capable or not to fix them. What you
>>> are proposing is to change the current default of no line-wrapping to
>>> line-wrap at getenv("COLUMNS"), isn't it?
>>
>> yes.
>
> OK, so to do this, I have to figure out where the pretty-printer is
> initialized in each FE. For C++ there are two different places:
>
> init_error() initializes cxx_pp

global_dc is the global diagnostic context -- that is used by almost
every front-end (except Ada and Fortran?)

cxx_pp is the C++ front-end-specific global pretty printer:  this is where
we dump C++ ASTs as strings. It is mostly used only by the various
tree -> string functions that are occasionally called from the debugger
or by the front-end for things like __PRETTY_FUNCTION__.  It is NOT
for diagnostic
purposes.  (it was never meant to survive so many releases.)

> whereas cxx_initialize_diagnostics seems to initialize a different
> pretty-printer for C++.

This is where we allocate a C++ pretty printer attached to the diagnostic
context passed in as a parameter.  It is this pretty printer that is used by
the diagnostic machinery.

>
> What is the difference between the two?
>
> Cheers,
>
> Manuel.

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/08/2012 12:13 PM, Manuel López-Ibáñez wrote:
> To focus on the issue at hand, and independently of whether any bugs
> exist or not, and whether I am capable or not to fix them. What you
> are proposing is to change the current default of no line-wrapping to
> line-wrap at getenv("COLUMNS"), isn't it?

I was just proposing to change the "80" to the value of 
getenv("COLUMNS").  If we don't currently use the value set there, let's 
not change that in this patch.

> And then, to use the same
> value for the max_width in the function printing the caret, is that
> correct?

Yes.

Jason

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

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

Attached a new version of the patch:

* It uses  the default cutoff as max_width, whatever it is (as
controlled by -fmessage-length).
* It uses the pretty-printer. The text cannot (should not) wrap
because we still print only max_width chars at most.
* It removes unnecessary prune patterns.

Bootstrapped and regression tested with languages=all,ada,obj-c++.

Cheers,

Manuel.

2012-04-09  Manuel López-Ibáñez  <manu@gcc.gnu.org>
	PR 24985
libstdc++-v3/
	* testsuite/lib/prune.exp: Handle caret.
libmudflap/
	* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
        * diagnostic.h (show_caret): Declare.
	(diagnostic_show_locus): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
        * input.c (read_line): New.
	(location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
	* pretty-print.h (pp_get_prefix): New.
	(pp_base_get_prefix): New.
        * common.opt (fdiagnostics-show-caret): New option.
	* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
        * lib/prune.exp: Add -fno-diagnostics-show-caret.

[-- Attachment #2: caret-diagnostics-20120409.diff --]
[-- Type: application/octet-stream, Size: 15362 bytes --]

Index: libstdc++-v3/testsuite/lib/prune.exp
===================================================================
--- libstdc++-v3/testsuite/lib/prune.exp	(revision 186103)
+++ libstdc++-v3/testsuite/lib/prune.exp	(working copy)
@@ -30,10 +30,16 @@ proc dg-prune-output { args } {
 }
 
 proc libstdc++-dg-prune { system text } {
     global additional_prunes
 
+#    send_user "Before:$text\n"
+
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+
     # Cygwin warns about -ffunction-sections
     regsub -all "(^|\n)\[^\n\]*: -ffunction-sections may affect debugging on some targets\[^\n\]*" $text "" text
 
     # Remove parts of warnings that refer to location of previous
     # definitions, etc as these confuse dejagnu
@@ -66,7 +72,8 @@ proc libstdc++-dg-prune { system text } 
 	    # Following regexp matches a complete line containing $p.
 	    regsub -all "(^|\n)\[^\n\]*$p\[^\n\]*" $text "" text
 	}
     }
 
+#    send_user "After:$text\n"
     return $text
 }
Index: libmudflap/testsuite/lib/libmudflap.exp
===================================================================
--- libmudflap/testsuite/lib/libmudflap.exp	(revision 186103)
+++ libmudflap/testsuite/lib/libmudflap.exp	(working copy)
@@ -296,10 +296,13 @@ proc libmudflap-dg-prune { system text }
     return $text
 }
 
 
 proc prune_gcc_output { text } {
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
     regsub -all {(^|\n)[^\n]*ld: warning: libgcc_s[^\n]*not found[^\n]*try using[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function.*pthread_create[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*the use of .pthread.*is deprecated[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*Dwarf Error:.*FORM value: 14[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function[^\n]*} $text "" text
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 186103)
+++ gcc/doc/invoke.texi	(working copy)
@@ -228,11 +228,11 @@ Objective-C and Objective-C++ Dialects}.
 
 @item Language Independent Options
 @xref{Language Independent Options,,Options to Control Diagnostic Messages Formatting}.
 @gccoptlist{-fmessage-length=@var{n}  @gol
 -fdiagnostics-show-location=@r{[}once@r{|}every-line@r{]}  @gol
--fno-diagnostics-show-option}
+-fno-diagnostics-show-option -fno-diagnostics-show-caret}
 
 @item Warning Options
 @xref{Warning Options,,Options to Request or Suppress Warnings}.
 @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -pedantic @gol
 -pedantic-errors @gol
@@ -2892,10 +2892,17 @@ a message which is too long to fit on a 
 By default, each diagnostic emitted includes text indicating the
 command-line option that directly controls the diagnostic (if such an
 option is known to the diagnostic machinery).  Specifying the
 @option{-fno-diagnostics-show-option} flag suppresses that behavior.
 
+@item -fno-diagnostics-show-caret
+@opindex fno-diagnostics-show-caret
+@opindex fdiagnostics-show-caret
+By default, each diagnostic emitted includes the original source line
+and a caret '^' indicating the column.  This option suppresses this
+information.
+
 @end table
 
 @node Warning Options
 @section Options to Request or Suppress Warnings
 @cindex options to control warnings
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 186103)
+++ gcc/diagnostic.c	(working copy)
@@ -98,10 +98,11 @@ diagnostic_initialize (diagnostic_contex
   context->warning_as_error_requested = false;
   context->n_opts = n_opts;
   context->classify_diagnostic = XNEWVEC (diagnostic_t, n_opts);
   for (i = 0; i < n_opts; i++)
     context->classify_diagnostic[i] = DK_UNSPECIFIED;
+  context->show_caret = false;
   context->show_option_requested = false;
   context->abort_on_error = false;
   context->show_column = false;
   context->pedantic_errors = false;
   context->permissive = false;
@@ -194,10 +195,86 @@ diagnostic_build_prefix (diagnostic_cont
      : context->show_column
      ? build_message_string ("%s:%d:%d: %s", s.file, s.line, s.column, text)
      : build_message_string ("%s:%d: %s", s.file, s.line, text));
 }
 
+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+void
+diagnostic_show_locus (diagnostic_context * context,
+		       const diagnostic_info *diagnostic)
+{
+  const char *line;
+  char *buffer;
+  expanded_location s;
+  int k, real_width, real_column;
+  int max_width;
+  const char *saved_prefix;
+
+  if (!context->show_caret
+      || diagnostic->location <= BUILTINS_LOCATION)
+    return;
+
+  s = expand_location(diagnostic->location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+    return;
+
+  for (k = real_column = 0; k < s.column && line[k] != '\0'; k++)
+    real_column += (line[k] == '\t') ? 8 : 1;
+
+  for (real_width = real_column; line[k] != '\0'; k++)
+    real_width += (line[k] == '\t') ? 8 : 1;
+
+
+  /* One minus to account for the leading empty space.  */
+  max_width = pp_line_cutoff (context->printer) - 1;
+  if (max_width <= 0)
+    max_width = INT_MAX;
+  else
+    {
+      int right_margin = 10;
+      /* If the line is longer than max_width and the column is too close
+	 to max_width, skip part of the line until it is not so close.  */
+      right_margin = max_width - right_margin;
+      if (real_width >= max_width && real_column > right_margin)
+	{
+	  for (k = real_column - right_margin; k > 0; line++)
+	    k -= (*line == '\t') ? 8 : 1;
+	  real_column = right_margin + k;
+	}
+    }
+
+  pp_newline (context->printer);
+  saved_prefix = pp_get_prefix (context->printer);
+  pp_set_prefix (context->printer, NULL);
+  pp_character (context->printer, ' ');
+  while (max_width > 0 && *line != '\0')
+    {
+      if (*line != '\t')
+	{
+	  max_width--;
+	  pp_character (context->printer, *line);
+	} 
+      else 
+	{
+	  /* TABs are 8 spaces.  */
+	  if (max_width - 8 <= 0) 
+	    break;
+	  pp_string (context->printer, "        ");
+	  max_width -= 8;
+	}
+      line++;
+    }
+  pp_newline (context->printer);
+  /* pp_printf does not implement %*c.  */
+  buffer = XALLOCAVEC (char, real_column + 3);
+  snprintf (buffer, real_column + 3, " %*c", real_column, '^');
+  pp_string (context->printer, buffer);
+  pp_set_prefix (context->printer, saved_prefix);
+}
+
 /* Take any action which is expected to happen after the diagnostic
    is written out.  This function does not always return.  */
 static void
 diagnostic_action_after_output (diagnostic_context *context,
 				diagnostic_info *diagnostic)
@@ -545,10 +622,11 @@ diagnostic_report_diagnostic (diagnostic
   diagnostic->message.x_data = &diagnostic->x_data;
   diagnostic->x_data = NULL;
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
+  diagnostic_show_locus (context, diagnostic);
   (*diagnostic_finalizer (context)) (context, diagnostic);
   pp_flush (context->printer);
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->x_data = NULL;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 186103)
+++ gcc/diagnostic.h	(working copy)
@@ -97,10 +97,14 @@ struct diagnostic_context
 
   /* For pragma push/pop.  */
   int *push_list;
   int n_push;
 
+  /* True if we should print the source line with a caret indicating
+     the location.  */
+  bool show_caret;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
@@ -252,10 +256,11 @@ extern diagnostic_context *global_dc;
 
 /* Diagnostic related functions.  */
 extern void diagnostic_initialize (diagnostic_context *, int);
 extern void diagnostic_finish (diagnostic_context *);
 extern void diagnostic_report_current_module (diagnostic_context *, location_t);
+extern void diagnostic_show_locus (diagnostic_context *, const diagnostic_info *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */,
Index: gcc/input.c
===================================================================
--- gcc/input.c	(revision 186103)
+++ gcc/input.c	(working copy)
@@ -48,10 +48,69 @@ expand_location (source_location loc)
     xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
 
   return xloc;
 }
 
+/* Reads one line from file into a static buffer.  */
+static const char *
+read_line (FILE *file)
+{
+  static char *string;
+  static size_t string_len;
+  size_t pos = 0;
+  char *ptr;
+
+  if (!string_len)
+    {
+      string_len = 200;
+      string = XNEWVEC (char, string_len);
+    }
+
+  while ((ptr = fgets (string + pos, string_len - pos, file)))
+    {
+      size_t len = strlen (string + pos);
+
+      if (string[pos + len - 1] == '\n')
+	{
+	  string[pos + len - 1] = 0;
+	  return string;
+	}
+      pos += len;
+      ptr = XNEWVEC (char, string_len * 2);
+      if (ptr)
+	{
+	  memcpy (ptr, string, pos);
+	  string = ptr;
+	  string_len += 2;
+	}
+      else
+	pos = 0;
+    }
+      
+  return pos ? string : NULL;
+}
+
+/* Return the physical source line that corresponds to xloc in a
+   buffer that is statically allocated.  The newline is replaced by
+   the null character.  */
+
+const char *
+location_get_source_line(expanded_location xloc)
+{
+  const char *buffer;
+  int lines = 1;
+  FILE *stream = xloc.file ? fopen (xloc.file, "r") : NULL;
+  if (!stream)
+    return NULL;
+
+  while ((buffer = read_line (stream)) && lines < xloc.line)
+    lines++;
+
+  fclose (stream);
+  return buffer;
+}
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
 /* Display a number as an integer multiple of either:
    - 1024, if said integer is >= to 10 K (in base 2)
Index: gcc/input.h
===================================================================
--- gcc/input.h	(revision 186103)
+++ gcc/input.h	(working copy)
@@ -36,10 +36,11 @@ extern GTY(()) struct line_maps *line_ta
    both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 extern char builtins_location_check[(BUILTINS_LOCATION
 				     < RESERVED_LOCATION_COUNT) ? 1 : -1];
 
 extern expanded_location expand_location (source_location);
+extern const char * location_get_source_line(expanded_location xloc);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
 typedef source_location location_t;
 
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c	(revision 186103)
+++ gcc/toplev.c	(working copy)
@@ -1167,10 +1167,12 @@ general_init (const char *argv0)
      finalizer -- for tokens resulting from macro macro expansion.  */
   diagnostic_finalizer (global_dc) = virt_loc_aware_diagnostic_finalizer;
   /* Set a default printer.  Language specific initializations will
      override it later.  */
   pp_format_decoder (global_dc->printer) = &default_tree_printer;
+  global_dc->show_caret
+    = global_options_init.x_flag_diagnostics_show_caret;
   global_dc->show_option_requested
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
   global_dc->internal_error = plugins_internal_error_function;
Index: gcc/pretty-print.h
===================================================================
--- gcc/pretty-print.h	(revision 186103)
+++ gcc/pretty-print.h	(working copy)
@@ -199,10 +199,13 @@ struct pretty_print_info
 };
 
 #define pp_set_line_maximum_length(PP, L) \
    pp_base_set_line_maximum_length (pp_base (PP), L)
 #define pp_set_prefix(PP, P)    pp_base_set_prefix (pp_base (PP), P)
+#define pp_get_prefix(PP)       pp_base_get_prefix (pp_base (PP))
+static inline const char *
+pp_base_get_prefix (const pretty_printer *pp) { return pp->prefix; }
 #define pp_destroy_prefix(PP)   pp_base_destroy_prefix (pp_base (PP))
 #define pp_remaining_character_count_for_line(PP) \
   pp_base_remaining_character_count_for_line (pp_base (PP))
 #define pp_clear_output_area(PP) \
   pp_base_clear_output_area (pp_base (PP))
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp	(revision 186103)
+++ gcc/testsuite/lib/prune.exp	(working copy)
@@ -15,10 +15,12 @@
 # along with GCC; see the file COPYING3.  If not see
 # <http://www.gnu.org/licenses/>.
 
 # Prune messages from gcc that aren't useful.
 
+set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS"
+
 proc prune_gcc_output { text } {
     #send_user "Before:$text\n"
 
     regsub -all "(^|\n)(\[^\n\]*: )?In ((static member |lambda )?function|member|method|(copy )?constructor|destructor|instantiation|substitution|program|subroutine|block-data)\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[^\n\]*(: )?At (top level|global scope):\[^\n\]*" $text "" text
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 186103)
+++ gcc/dwarf2out.c	(working copy)
@@ -18367,10 +18367,11 @@ gen_producer_string (void)
       case OPT_grecord_gcc_switches:
       case OPT_gno_record_gcc_switches:
       case OPT__output_pch_:
       case OPT_fdiagnostics_show_location_:
       case OPT_fdiagnostics_show_option:
+      case OPT_fdiagnostics_show_caret:
       case OPT_fverbose_asm:
       case OPT____:
       case OPT__sysroot_:
       case OPT_nostdinc:
       case OPT_nostdinc__:
Index: gcc/opts.c
===================================================================
--- gcc/opts.c	(revision 186103)
+++ gcc/opts.c	(working copy)
@@ -1497,10 +1497,14 @@ common_handle_option (struct gcc_options
       break;
 
     case OPT_fdiagnostics_show_location_:
       diagnostic_prefixing_rule (dc) = (diagnostic_prefixing_rule_t) value;
       break;
+ 
+    case OPT_fdiagnostics_show_caret:
+      dc->show_caret = value;
+      break;
 
     case OPT_fdiagnostics_show_option:
       dc->show_option_requested = value;
       break;
 
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 186103)
+++ gcc/common.opt	(working copy)
@@ -997,10 +997,14 @@ EnumValue
 Enum(diagnostic_prefixing_rule) String(once) Value(DIAGNOSTICS_SHOW_PREFIX_ONCE)
 
 EnumValue
 Enum(diagnostic_prefixing_rule) String(every-line) Value(DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE)
 
+fdiagnostics-show-caret
+Common Var(flag_diagnostics_show_caret) Init(1)
+Show the source line with a caret indicating the column
+
 fdiagnostics-show-option
 Common Var(flag_diagnostics_show_option) Init(1)
 Amend appropriate diagnostic messages with the command line option that controls them
 
 fdisable-

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/09/2012 04:01 PM, Manuel López-Ibáñez wrote:
> * It uses  the default cutoff as max_width, whatever it is (as
> controlled by -fmessage-length).
> * It uses the pretty-printer. The text cannot (should not) wrap
> because we still print only max_width chars at most.

Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still 
want to use COLUMNS to limit how much of the source we print.

Jason

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

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

On 10 April 2012 00:28, Jason Merrill <jason@redhat.com> wrote:
> On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
>>
>> * It uses  the default cutoff as max_width, whatever it is (as
>> controlled by -fmessage-length).
>> * It uses the pretty-printer. The text cannot (should not) wrap
>> because we still print only max_width chars at most.
>
>
> Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
> to use COLUMNS to limit how much of the source we print.

Like this?

2012-04-05  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR 24985
libstdc++-v3/
	* testsuite/lib/prune.exp: Handle caret.
libmudflap/
	* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
        * diagnostic.h (show_caret): Declare.
	(caret_max_width): Declare.
	(diagnostic_show_locus): Declare.
	(diagnostic_set_caret_max_width): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
	(getenv_columns): New.
	(diagnostic_set_caret_max_width): New.
        * input.c (read_line): New.
	(location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
	* pretty-print.h (pp_get_prefix): New.
	(pp_base_get_prefix): New.
        * common.opt (fdiagnostics-show-caret): New option.
	* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
        * lib/prune.exp: Add -fno-diagnostics-show-caret.

[-- Attachment #2: caret-diagnostics-20120410.diff --]
[-- Type: application/octet-stream, Size: 17275 bytes --]

Index: libstdc++-v3/testsuite/lib/prune.exp
===================================================================
--- libstdc++-v3/testsuite/lib/prune.exp	(revision 186103)
+++ libstdc++-v3/testsuite/lib/prune.exp	(working copy)
@@ -30,10 +30,16 @@ proc dg-prune-output { args } {
 }
 
 proc libstdc++-dg-prune { system text } {
     global additional_prunes
 
+#    send_user "Before:$text\n"
+
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+
     # Cygwin warns about -ffunction-sections
     regsub -all "(^|\n)\[^\n\]*: -ffunction-sections may affect debugging on some targets\[^\n\]*" $text "" text
 
     # Remove parts of warnings that refer to location of previous
     # definitions, etc as these confuse dejagnu
@@ -66,7 +72,8 @@ proc libstdc++-dg-prune { system text } 
 	    # Following regexp matches a complete line containing $p.
 	    regsub -all "(^|\n)\[^\n\]*$p\[^\n\]*" $text "" text
 	}
     }
 
+#    send_user "After:$text\n"
     return $text
 }
Index: libmudflap/testsuite/lib/libmudflap.exp
===================================================================
--- libmudflap/testsuite/lib/libmudflap.exp	(revision 186103)
+++ libmudflap/testsuite/lib/libmudflap.exp	(working copy)
@@ -296,10 +296,13 @@ proc libmudflap-dg-prune { system text }
     return $text
 }
 
 
 proc prune_gcc_output { text } {
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
     regsub -all {(^|\n)[^\n]*ld: warning: libgcc_s[^\n]*not found[^\n]*try using[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function.*pthread_create[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*the use of .pthread.*is deprecated[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*Dwarf Error:.*FORM value: 14[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function[^\n]*} $text "" text
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 186103)
+++ gcc/doc/invoke.texi	(working copy)
@@ -228,11 +228,11 @@ Objective-C and Objective-C++ Dialects}.
 
 @item Language Independent Options
 @xref{Language Independent Options,,Options to Control Diagnostic Messages Formatting}.
 @gccoptlist{-fmessage-length=@var{n}  @gol
 -fdiagnostics-show-location=@r{[}once@r{|}every-line@r{]}  @gol
--fno-diagnostics-show-option}
+-fno-diagnostics-show-option -fno-diagnostics-show-caret}
 
 @item Warning Options
 @xref{Warning Options,,Options to Request or Suppress Warnings}.
 @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -pedantic @gol
 -pedantic-errors @gol
@@ -2892,10 +2892,17 @@ a message which is too long to fit on a 
 By default, each diagnostic emitted includes text indicating the
 command-line option that directly controls the diagnostic (if such an
 option is known to the diagnostic machinery).  Specifying the
 @option{-fno-diagnostics-show-option} flag suppresses that behavior.
 
+@item -fno-diagnostics-show-caret
+@opindex fno-diagnostics-show-caret
+@opindex fdiagnostics-show-caret
+By default, each diagnostic emitted includes the original source line
+and a caret '^' indicating the column.  This option suppresses this
+information.
+
 @end table
 
 @node Warning Options
 @section Options to Request or Suppress Warnings
 @cindex options to control warnings
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 186103)
+++ gcc/diagnostic.c	(working copy)
@@ -76,10 +76,34 @@ file_name_as_prefix (const char *f)
   return build_message_string ("%s: ", f);
 }
 
 
 \f
+/* Return the value of the getenv("COLUMNS") as an integer. If the
+   value is not set to a positive integer, then return INT_MAX.  */
+static int
+getenv_columns (void)
+{
+  const char * s = getenv ("COLUMNS");
+  if (s != NULL) {
+    int n = atoi (s);
+    if (n > 0)
+      return n;
+  }
+  return INT_MAX;
+}
+
+/* Set caret_max_width to value.  */
+void
+diagnostic_set_caret_max_width (diagnostic_context *context, int value)
+{
+  /* One minus to account for the leading empty space.  */
+  context->caret_max_width = value
+    ? value - 1 : (isatty (fileno (context->printer->buffer->stream))
+		   ? getenv_columns () - 1: INT_MAX);
+}
+
 /* Initialize the diagnostic message outputting machinery.  */
 void
 diagnostic_initialize (diagnostic_context *context, int n_opts)
 {
   int i;
@@ -98,10 +122,12 @@ diagnostic_initialize (diagnostic_contex
   context->warning_as_error_requested = false;
   context->n_opts = n_opts;
   context->classify_diagnostic = XNEWVEC (diagnostic_t, n_opts);
   for (i = 0; i < n_opts; i++)
     context->classify_diagnostic[i] = DK_UNSPECIFIED;
+  context->show_caret = false;
+  diagnostic_set_caret_max_width (context, pp_line_cutoff (context->printer));
   context->show_option_requested = false;
   context->abort_on_error = false;
   context->show_column = false;
   context->pedantic_errors = false;
   context->permissive = false;
@@ -194,10 +220,82 @@ diagnostic_build_prefix (diagnostic_cont
      : context->show_column
      ? build_message_string ("%s:%d:%d: %s", s.file, s.line, s.column, text)
      : build_message_string ("%s:%d: %s", s.file, s.line, text));
 }
 
+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+void
+diagnostic_show_locus (diagnostic_context * context,
+		       const diagnostic_info *diagnostic)
+{
+  const char *line;
+  char *buffer;
+  expanded_location s;
+  int k, real_width, real_column;
+  int max_width;
+  const char *saved_prefix;
+  int right_margin = 10;
+
+  if (!context->show_caret
+      || diagnostic->location <= BUILTINS_LOCATION)
+    return;
+
+  s = expand_location(diagnostic->location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+    return;
+
+  for (k = real_column = 0; k < s.column && line[k] != '\0'; k++)
+    real_column += (line[k] == '\t') ? 8 : 1;
+
+  for (real_width = real_column; line[k] != '\0'; k++)
+    real_width += (line[k] == '\t') ? 8 : 1;
+
+  max_width = context->caret_max_width;
+  if (max_width <= 0)
+    max_width = INT_MAX;
+
+  /* If the line is longer than max_width and the column is too close
+     to max_width, skip part of the line until it is not so close.  */
+  right_margin = max_width - right_margin;
+  if (real_width >= max_width && real_column > right_margin)
+    {
+      for (k = real_column - right_margin; k > 0; line++)
+	k -= (*line == '\t') ? 8 : 1;
+      real_column = right_margin + k;
+    }
+
+  pp_newline (context->printer);
+  saved_prefix = pp_get_prefix (context->printer);
+  pp_set_prefix (context->printer, NULL);
+  pp_character (context->printer, ' ');
+  while (max_width > 0 && *line != '\0')
+    {
+      if (*line != '\t')
+	{
+	  max_width--;
+	  pp_character (context->printer, *line);
+	} 
+      else 
+	{
+	  /* TABs are 8 spaces.  */
+	  if (max_width - 8 <= 0) 
+	    break;
+	  pp_string (context->printer, "        ");
+	  max_width -= 8;
+	}
+      line++;
+    }
+  pp_newline (context->printer);
+  /* pp_printf does not implement %*c.  */
+  buffer = XALLOCAVEC (char, real_column + 3);
+  snprintf (buffer, real_column + 3, " %*c", real_column, '^');
+  pp_string (context->printer, buffer);
+  pp_set_prefix (context->printer, saved_prefix);
+}
+
 /* Take any action which is expected to happen after the diagnostic
    is written out.  This function does not always return.  */
 static void
 diagnostic_action_after_output (diagnostic_context *context,
 				diagnostic_info *diagnostic)
@@ -545,10 +643,11 @@ diagnostic_report_diagnostic (diagnostic
   diagnostic->message.x_data = &diagnostic->x_data;
   diagnostic->x_data = NULL;
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
+  diagnostic_show_locus (context, diagnostic);
   (*diagnostic_finalizer (context)) (context, diagnostic);
   pp_flush (context->printer);
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->x_data = NULL;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 186103)
+++ gcc/diagnostic.h	(working copy)
@@ -97,10 +97,17 @@ struct diagnostic_context
 
   /* For pragma push/pop.  */
   int *push_list;
   int n_push;
 
+  /* True if we should print the source line with a caret indicating
+     the location.  */
+  bool show_caret;
+
+  /* Maximum width of the source line printed.  */
+  int caret_max_width;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
@@ -252,10 +259,11 @@ extern diagnostic_context *global_dc;
 
 /* Diagnostic related functions.  */
 extern void diagnostic_initialize (diagnostic_context *, int);
 extern void diagnostic_finish (diagnostic_context *);
 extern void diagnostic_report_current_module (diagnostic_context *, location_t);
+extern void diagnostic_show_locus (diagnostic_context *, const diagnostic_info *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */,
@@ -273,10 +281,12 @@ extern void diagnostic_set_info_translat
      ATTRIBUTE_GCC_DIAG(2,0);
 #endif
 extern char *diagnostic_build_prefix (diagnostic_context *, diagnostic_info *);
 void default_diagnostic_starter (diagnostic_context *, diagnostic_info *);
 void default_diagnostic_finalizer (diagnostic_context *, diagnostic_info *);
+void diagnostic_set_caret_max_width (diagnostic_context *, int);
+
 
 /* Pure text formatting support functions.  */
 extern char *file_name_as_prefix (const char *);
 
 #endif /* ! GCC_DIAGNOSTIC_H */
Index: gcc/input.c
===================================================================
--- gcc/input.c	(revision 186103)
+++ gcc/input.c	(working copy)
@@ -48,10 +48,69 @@ expand_location (source_location loc)
     xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
 
   return xloc;
 }
 
+/* Reads one line from file into a static buffer.  */
+static const char *
+read_line (FILE *file)
+{
+  static char *string;
+  static size_t string_len;
+  size_t pos = 0;
+  char *ptr;
+
+  if (!string_len)
+    {
+      string_len = 200;
+      string = XNEWVEC (char, string_len);
+    }
+
+  while ((ptr = fgets (string + pos, string_len - pos, file)))
+    {
+      size_t len = strlen (string + pos);
+
+      if (string[pos + len - 1] == '\n')
+	{
+	  string[pos + len - 1] = 0;
+	  return string;
+	}
+      pos += len;
+      ptr = XNEWVEC (char, string_len * 2);
+      if (ptr)
+	{
+	  memcpy (ptr, string, pos);
+	  string = ptr;
+	  string_len += 2;
+	}
+      else
+	pos = 0;
+    }
+      
+  return pos ? string : NULL;
+}
+
+/* Return the physical source line that corresponds to xloc in a
+   buffer that is statically allocated.  The newline is replaced by
+   the null character.  */
+
+const char *
+location_get_source_line(expanded_location xloc)
+{
+  const char *buffer;
+  int lines = 1;
+  FILE *stream = xloc.file ? fopen (xloc.file, "r") : NULL;
+  if (!stream)
+    return NULL;
+
+  while ((buffer = read_line (stream)) && lines < xloc.line)
+    lines++;
+
+  fclose (stream);
+  return buffer;
+}
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
 /* Display a number as an integer multiple of either:
    - 1024, if said integer is >= to 10 K (in base 2)
Index: gcc/input.h
===================================================================
--- gcc/input.h	(revision 186103)
+++ gcc/input.h	(working copy)
@@ -36,10 +36,11 @@ extern GTY(()) struct line_maps *line_ta
    both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 extern char builtins_location_check[(BUILTINS_LOCATION
 				     < RESERVED_LOCATION_COUNT) ? 1 : -1];
 
 extern expanded_location expand_location (source_location);
+extern const char * location_get_source_line(expanded_location xloc);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
 typedef source_location location_t;
 
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c	(revision 186103)
+++ gcc/toplev.c	(working copy)
@@ -1167,10 +1167,12 @@ general_init (const char *argv0)
      finalizer -- for tokens resulting from macro macro expansion.  */
   diagnostic_finalizer (global_dc) = virt_loc_aware_diagnostic_finalizer;
   /* Set a default printer.  Language specific initializations will
      override it later.  */
   pp_format_decoder (global_dc->printer) = &default_tree_printer;
+  global_dc->show_caret
+    = global_options_init.x_flag_diagnostics_show_caret;
   global_dc->show_option_requested
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
   global_dc->internal_error = plugins_internal_error_function;
Index: gcc/pretty-print.h
===================================================================
--- gcc/pretty-print.h	(revision 186103)
+++ gcc/pretty-print.h	(working copy)
@@ -199,10 +199,13 @@ struct pretty_print_info
 };
 
 #define pp_set_line_maximum_length(PP, L) \
    pp_base_set_line_maximum_length (pp_base (PP), L)
 #define pp_set_prefix(PP, P)    pp_base_set_prefix (pp_base (PP), P)
+#define pp_get_prefix(PP)       pp_base_get_prefix (pp_base (PP))
+static inline const char *
+pp_base_get_prefix (const pretty_printer *pp) { return pp->prefix; }
 #define pp_destroy_prefix(PP)   pp_base_destroy_prefix (pp_base (PP))
 #define pp_remaining_character_count_for_line(PP) \
   pp_base_remaining_character_count_for_line (pp_base (PP))
 #define pp_clear_output_area(PP) \
   pp_base_clear_output_area (pp_base (PP))
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp	(revision 186103)
+++ gcc/testsuite/lib/prune.exp	(working copy)
@@ -15,10 +15,12 @@
 # along with GCC; see the file COPYING3.  If not see
 # <http://www.gnu.org/licenses/>.
 
 # Prune messages from gcc that aren't useful.
 
+set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS"
+
 proc prune_gcc_output { text } {
     #send_user "Before:$text\n"
 
     regsub -all "(^|\n)(\[^\n\]*: )?In ((static member |lambda )?function|member|method|(copy )?constructor|destructor|instantiation|substitution|program|subroutine|block-data)\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[^\n\]*(: )?At (top level|global scope):\[^\n\]*" $text "" text
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 186103)
+++ gcc/dwarf2out.c	(working copy)
@@ -18367,10 +18367,11 @@ gen_producer_string (void)
       case OPT_grecord_gcc_switches:
       case OPT_gno_record_gcc_switches:
       case OPT__output_pch_:
       case OPT_fdiagnostics_show_location_:
       case OPT_fdiagnostics_show_option:
+      case OPT_fdiagnostics_show_caret:
       case OPT_fverbose_asm:
       case OPT____:
       case OPT__sysroot_:
       case OPT_nostdinc:
       case OPT_nostdinc__:
Index: gcc/opts.c
===================================================================
--- gcc/opts.c	(revision 186103)
+++ gcc/opts.c	(working copy)
@@ -1497,10 +1497,14 @@ common_handle_option (struct gcc_options
       break;
 
     case OPT_fdiagnostics_show_location_:
       diagnostic_prefixing_rule (dc) = (diagnostic_prefixing_rule_t) value;
       break;
+ 
+    case OPT_fdiagnostics_show_caret:
+      dc->show_caret = value;
+      break;
 
     case OPT_fdiagnostics_show_option:
       dc->show_option_requested = value;
       break;
 
@@ -1537,10 +1541,11 @@ common_handle_option (struct gcc_options
 	(&opts->x_flag_instrument_functions_exclude_files, arg);
       break;
 
     case OPT_fmessage_length_:
       pp_set_line_maximum_length (dc->printer, value);
+      diagnostic_set_caret_max_width (dc, value);
       break;
 
     case OPT_fpack_struct_:
       if (value <= 0 || (value & (value - 1)) || value > 16)
 	error_at (loc,
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 186103)
+++ gcc/common.opt	(working copy)
@@ -997,10 +997,14 @@ EnumValue
 Enum(diagnostic_prefixing_rule) String(once) Value(DIAGNOSTICS_SHOW_PREFIX_ONCE)
 
 EnumValue
 Enum(diagnostic_prefixing_rule) String(every-line) Value(DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE)
 
+fdiagnostics-show-caret
+Common Var(flag_diagnostics_show_caret) Init(1)
+Show the source line with a caret indicating the column
+
 fdiagnostics-show-option
 Common Var(flag_diagnostics_show_option) Init(1)
 Amend appropriate diagnostic messages with the command line option that controls them
 
 fdisable-

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 10 April 2012 00:28, Jason Merrill <jason@redhat.com> wrote:
>> On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
>>>
>>> * It uses  the default cutoff as max_width, whatever it is (as
>>> controlled by -fmessage-length).
>>> * It uses the pretty-printer. The text cannot (should not) wrap
>>> because we still print only max_width chars at most.
>>
>>
>> Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
>> to use COLUMNS to limit how much of the source we print.
>
> Like this?

There is a novelty in this new version that I don't think we discussed
before: automatic expansion of tabs to 8 hard space characters.  That
number should not be hardcoded as there is no reason to believe a tab
character always expands to 8 space characters.  You should check
the environment first; if not present the default expansion number should
be a symbolic constant as opposed to a magic number sprinkled all over the
places.  I would also encourage you to introduce more abstraction to
reduce the size of diagnostic_show_locus.

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:
> +  max_width = context->caret_max_width;
> +  if (max_width<= 0)
> +    max_width = INT_MAX;

I don't think we need the test here; diagnostic_set_caret_max_width 
should make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.

On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:
> There is a novelty in this new version that I don't think we discussed
> before: automatic expansion of tabs to 8 hard space characters.  That
> number should not be hardcoded as there is no reason to believe a tab
> character always expands to 8 space characters.  You should check
> the environment first; if not present the default expansion number should
> be a symbolic constant as opposed to a magic number sprinkled all over the
> places.

Hmm.  I don't know if there's any reliable way to query tab stops; the 
"it" termcap/terminfo capability tells you what it was originally 
(presumably 8), but it might have changed.

Jason

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 10 April 2012 20:52, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
> On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
>> On 10 April 2012 00:28, Jason Merrill <jason@redhat.com> wrote:
>>> On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
>>>>
>>>> * It uses  the default cutoff as max_width, whatever it is (as
>>>> controlled by -fmessage-length).
>>>> * It uses the pretty-printer. The text cannot (should not) wrap
>>>> because we still print only max_width chars at most.
>>>
>>>
>>> Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
>>> to use COLUMNS to limit how much of the source we print.
>>
>> Like this?
>
> There is a novelty in this new version that I don't think we discussed
> before: automatic expansion of tabs to 8 hard space characters.  That
> number should not be hardcoded as there is no reason to believe a tab
> character always expands to 8 space characters.  You should check
> the environment first; if not present the default expansion number should
> be a symbolic constant as opposed to a magic number sprinkled all over the
> places.  I would also encourage you to introduce more abstraction to
> reduce the size of diagnostic_show_locus.

There is no novelty, it has been there from the beginning, and now I
realize it was a mistake to do the extra work to implement this.

The GCS says: "Calculate column numbers assuming that space and all
ASCII printing characters have equal width, and assuming tab stops
every 8 columns"
http://www.gnu.org/prep/standards/standards.html#Errors

So, in fact, libcpp is buggy for not implementing this (as can be seen
in emacs). If/When libcpp is fixed, the column info will be correct
for tabs. And then, one may care about printing tabs as anything
different from a single space. But until then, a single space is a
good as anything. In fact this is what is done in c-ppoutput.c.

As can be seen below, this simplifies a lot the function, so I
appreciate the suggestion. If you have suggestions for removing other
code, I will be happy to implement them. The less code, the less
potential for bugs.

Cheers,

Manuel.

+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+void
+diagnostic_show_locus (diagnostic_context * context,
+                      const diagnostic_info *diagnostic)
+{
+  const char *line;
+  char *buffer;
+  expanded_location s;
+  int real_width;
+  int max_width;
+  const char *saved_prefix;
+  int right_margin = 10;
+
+  if (!context->show_caret
+      || diagnostic->location <= BUILTINS_LOCATION)
+    return;
+
+  s = expand_location(diagnostic->location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+    return;
+
+  real_width = strlen (line);
+
+  max_width = context->caret_max_width;
+  if (max_width <= 0)
+    max_width = INT_MAX;
+
+  /* If the line is longer than max_width and the column is too close
+     to max_width, skip part of the line until it is not so close.  */
+  right_margin = MIN(real_width - s.column, right_margin);
+  right_margin = max_width - right_margin;
+  if (real_width >= max_width && s.column > right_margin)
+    {
+      line += s.column - right_margin;
+      s.column = right_margin;
+    }
+
+  pp_newline (context->printer);
+  saved_prefix = pp_get_prefix (context->printer);
+  pp_set_prefix (context->printer, NULL);
+  pp_character (context->printer, ' ');
+  while (max_width > 0 && *line != '\0')
+    {
+      char c = *line == '\t' ? ' ' : *line;
+      pp_character (context->printer, c);
+      max_width--;
+      line++;
+    }
+  pp_newline (context->printer);
+  /* pp_printf does not implement %*c.  */
+  buffer = XALLOCAVEC (char, s.column + 3);
+  snprintf (buffer, s.column + 3, " %*c", s.column, '^');
+  pp_string (context->printer, buffer);
+  pp_set_prefix (context->printer, saved_prefix);
+}
+

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 10 April 2012 21:31, Jason Merrill <jason@redhat.com> wrote:
> On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:
>>
>> +  max_width = context->caret_max_width;
>> +  if (max_width<= 0)
>> +    max_width = INT_MAX;
>
>
> I don't think we need the test here; diagnostic_set_caret_max_width should
> make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.

It was just a bit of defensive programming, since nothing stops anyone
to set the value directly. But I will remove it.

> On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:
>>
>> There is a novelty in this new version that I don't think we discussed
>> before: automatic expansion of tabs to 8 hard space characters.  That
>> number should not be hardcoded as there is no reason to believe a tab
>> character always expands to 8 space characters.  You should check
>> the environment first; if not present the default expansion number should
>> be a symbolic constant as opposed to a magic number sprinkled all over the
>> places.
>
>
> Hmm.  I don't know if there's any reliable way to query tab stops; the "it"
> termcap/terminfo capability tells you what it was originally (presumably 8),
> but it might have changed.

No idea either, but it is in fact easier to print tabs a single
spaces. This simplifies the code a lot, as pointed by Gabriel. So if
you also prefer the simpler version, I am fine with committing that
one (it also saves space in the output).

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

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

On 10 April 2012 21:41, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 10 April 2012 21:31, Jason Merrill <jason@redhat.com> wrote:
>> On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:
>>>
>>> +  max_width = context->caret_max_width;
>>> +  if (max_width<= 0)
>>> +    max_width = INT_MAX;
>>
>>
>> I don't think we need the test here; diagnostic_set_caret_max_width should
>> make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.
>
> It was just a bit of defensive programming, since nothing stops anyone
> to set the value directly. But I will remove it.
>
>> On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:
>>>
>>> There is a novelty in this new version that I don't think we discussed
>>> before: automatic expansion of tabs to 8 hard space characters.  That
>>> number should not be hardcoded as there is no reason to believe a tab
>>> character always expands to 8 space characters.  You should check
>>> the environment first; if not present the default expansion number should
>>> be a symbolic constant as opposed to a magic number sprinkled all over the
>>> places.
>>
>>
>> Hmm.  I don't know if there's any reliable way to query tab stops; the "it"
>> termcap/terminfo capability tells you what it was originally (presumably 8),
>> but it might have changed.
>
> No idea either, but it is in fact easier to print tabs a single
> spaces. This simplifies the code a lot, as pointed by Gabriel. So if
> you also prefer the simpler version, I am fine with committing that
> one (it also saves space in the output).

A new version of the patch. It removes the special handling of tabs.
And abstracts out the adjusting of the line as Gabriel suggested. I am
not sure what else could be abstracted out. The function is much
shorter than other functions in diagnostic.c.

This patch builds and I am running a full boostrap+check now.

OK if it passes?

Cheers,

Manuel.


2012-04-05  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR 24985
libstdc++-v3/
	* testsuite/lib/prune.exp: Handle caret.
libmudflap/
	* testsuite/lib/libmudflap.exp: Handle caret.
gcc/
        * diagnostic.h (show_caret): Declare.
	(caret_max_width): Declare.
	(diagnostic_show_locus): Declare.
        * diagnostic.c (diagnostic_initialize): Initialize to false.
        (diagnostic_show_locus): New.
        (diagnostic_report_diagnostic): Call it.
	(getenv_columns): New.
	(adjust_line): New.
	(diagnostic_set_caret_max_width): New.
        * input.c (read_line): New.
	(location_get_source_line): New.
        * input.h (location_get_source_line): Declare.
        * toplev.c (general_init): Initialize show_caret from options.
        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
        * opts.c (common_handle_option): Likewise.
	* pretty-print.h (pp_get_prefix): New.
	(pp_base_get_prefix): New.
        * common.opt (fdiagnostics-show-caret): New option.
	* doc/invoke.texi (fdiagnostics-show-caret): Document it.
testsuite/
        * lib/prune.exp: Add -fno-diagnostics-show-caret.

[-- Attachment #2: caret-diagnostics-20120411.diff --]
[-- Type: application/octet-stream, Size: 17310 bytes --]

Index: libstdc++-v3/testsuite/lib/prune.exp
===================================================================
--- libstdc++-v3/testsuite/lib/prune.exp	(revision 186103)
+++ libstdc++-v3/testsuite/lib/prune.exp	(working copy)
@@ -30,10 +30,16 @@ proc dg-prune-output { args } {
 }
 
 proc libstdc++-dg-prune { system text } {
     global additional_prunes
 
+#    send_user "Before:$text\n"
+
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
+
     # Cygwin warns about -ffunction-sections
     regsub -all "(^|\n)\[^\n\]*: -ffunction-sections may affect debugging on some targets\[^\n\]*" $text "" text
 
     # Remove parts of warnings that refer to location of previous
     # definitions, etc as these confuse dejagnu
@@ -66,7 +72,8 @@ proc libstdc++-dg-prune { system text } 
 	    # Following regexp matches a complete line containing $p.
 	    regsub -all "(^|\n)\[^\n\]*$p\[^\n\]*" $text "" text
 	}
     }
 
+#    send_user "After:$text\n"
     return $text
 }
Index: libmudflap/testsuite/lib/libmudflap.exp
===================================================================
--- libmudflap/testsuite/lib/libmudflap.exp	(revision 186103)
+++ libmudflap/testsuite/lib/libmudflap.exp	(working copy)
@@ -296,10 +296,13 @@ proc libmudflap-dg-prune { system text }
     return $text
 }
 
 
 proc prune_gcc_output { text } {
+    # Ignore caret diagnostics. Unfortunately dejaGNU trims leading
+    # spaces, so one cannot rely on them being present.
+    regsub -all "(^|\n)\[^\n\]+\n *\\^\n" $text "\n" text
     regsub -all {(^|\n)[^\n]*ld: warning: libgcc_s[^\n]*not found[^\n]*try using[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function.*pthread_create[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*the use of .pthread.*is deprecated[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*Dwarf Error:.*FORM value: 14[^\n]*} $text "" text
     regsub -all {(^|\n)[^\n]*In function[^\n]*} $text "" text
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 186103)
+++ gcc/doc/invoke.texi	(working copy)
@@ -228,11 +228,11 @@ Objective-C and Objective-C++ Dialects}.
 
 @item Language Independent Options
 @xref{Language Independent Options,,Options to Control Diagnostic Messages Formatting}.
 @gccoptlist{-fmessage-length=@var{n}  @gol
 -fdiagnostics-show-location=@r{[}once@r{|}every-line@r{]}  @gol
--fno-diagnostics-show-option}
+-fno-diagnostics-show-option -fno-diagnostics-show-caret}
 
 @item Warning Options
 @xref{Warning Options,,Options to Request or Suppress Warnings}.
 @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -pedantic @gol
 -pedantic-errors @gol
@@ -2892,10 +2892,17 @@ a message which is too long to fit on a 
 By default, each diagnostic emitted includes text indicating the
 command-line option that directly controls the diagnostic (if such an
 option is known to the diagnostic machinery).  Specifying the
 @option{-fno-diagnostics-show-option} flag suppresses that behavior.
 
+@item -fno-diagnostics-show-caret
+@opindex fno-diagnostics-show-caret
+@opindex fdiagnostics-show-caret
+By default, each diagnostic emitted includes the original source line
+and a caret '^' indicating the column.  This option suppresses this
+information.
+
 @end table
 
 @node Warning Options
 @section Options to Request or Suppress Warnings
 @cindex options to control warnings
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 186103)
+++ gcc/diagnostic.c	(working copy)
@@ -76,10 +76,39 @@ file_name_as_prefix (const char *f)
   return build_message_string ("%s: ", f);
 }
 
 
 \f
+/* Return the value of the getenv("COLUMNS") as an integer. If the
+   value is not set to a positive integer, then return INT_MAX.  */
+static int
+getenv_columns (void)
+{
+  const char * s = getenv ("COLUMNS");
+  if (s != NULL) {
+    int n = atoi (s);
+    if (n > 0)
+      return n;
+  }
+  return INT_MAX;
+}
+
+/* Set caret_max_width to value.  */
+void
+diagnostic_set_caret_max_width (diagnostic_context *context, int value)
+{
+  /* One minus to account for the leading empty space.  */
+  value = value ? value - 1 
+    : (isatty (fileno (context->printer->buffer->stream))
+       ? getenv_columns () - 1: INT_MAX);
+  
+  if (value <= 0) 
+    value = INT_MAX;
+
+  context->caret_max_width = value;
+}
+
 /* Initialize the diagnostic message outputting machinery.  */
 void
 diagnostic_initialize (diagnostic_context *context, int n_opts)
 {
   int i;
@@ -98,10 +127,12 @@ diagnostic_initialize (diagnostic_contex
   context->warning_as_error_requested = false;
   context->n_opts = n_opts;
   context->classify_diagnostic = XNEWVEC (diagnostic_t, n_opts);
   for (i = 0; i < n_opts; i++)
     context->classify_diagnostic[i] = DK_UNSPECIFIED;
+  context->show_caret = false;
+  diagnostic_set_caret_max_width (context, pp_line_cutoff (context->printer));
   context->show_option_requested = false;
   context->abort_on_error = false;
   context->show_column = false;
   context->pedantic_errors = false;
   context->permissive = false;
@@ -194,10 +225,76 @@ diagnostic_build_prefix (diagnostic_cont
      : context->show_column
      ? build_message_string ("%s:%d:%d: %s", s.file, s.line, s.column, text)
      : build_message_string ("%s:%d: %s", s.file, s.line, text));
 }
 
+/* If LINE is longer than MAX_WIDTH, and COLUMN is not smaller than
+   MAX_WIDTH by some margin, then adjust the start of the line such
+   that the COLUMN is smaller than MAX_WIDTH minus the margin.  The
+   margin is either 10 characters or the difference between the column
+   and the length of the line, whatever is smaller.  */
+static const char *
+adjust_line (const char *line, int max_width, int *column_p)
+{
+  int right_margin = 10;
+  int line_width = strlen (line);
+  int column = *column_p;
+
+  right_margin = MIN (line_width - column, right_margin);
+  right_margin = max_width - right_margin;
+  if (line_width >= max_width && column > right_margin)
+    {
+      line += column - right_margin;
+      *column_p = right_margin;
+    }
+  return line;
+}
+
+/* Print the physical source line corresponding to the location of
+   this diagnostics, and a caret indicating the precise column.  */
+void
+diagnostic_show_locus (diagnostic_context * context,
+		       const diagnostic_info *diagnostic)
+{
+  const char *line;
+  char *buffer;
+  expanded_location s;
+  int max_width;
+  const char *saved_prefix;
+
+
+  if (!context->show_caret
+      || diagnostic->location <= BUILTINS_LOCATION)
+    return;
+
+  s = expand_location(diagnostic->location);
+  line = location_get_source_line (s);
+  if (line == NULL)
+    return;
+
+  max_width = context->caret_max_width;
+  line = adjust_line (line, max_width, &(s.column));
+
+  pp_newline (context->printer);
+  saved_prefix = pp_get_prefix (context->printer);
+  pp_set_prefix (context->printer, NULL);
+  pp_character (context->printer, ' ');
+  while (max_width > 0 && *line != '\0')
+    {
+      char c = *line == '\t' ? ' ' : *line;
+      pp_character (context->printer, c);
+      max_width--;
+      line++;
+    }
+  pp_newline (context->printer);
+  /* pp_printf does not implement %*c.  */
+  buffer = XALLOCAVEC (char, s.column + 3);
+  snprintf (buffer, s.column + 3, " %*c", s.column, '^');
+  pp_string (context->printer, buffer);
+  pp_set_prefix (context->printer, saved_prefix);
+}
+
 /* Take any action which is expected to happen after the diagnostic
    is written out.  This function does not always return.  */
 static void
 diagnostic_action_after_output (diagnostic_context *context,
 				diagnostic_info *diagnostic)
@@ -545,10 +642,11 @@ diagnostic_report_diagnostic (diagnostic
   diagnostic->message.x_data = &diagnostic->x_data;
   diagnostic->x_data = NULL;
   pp_format (context->printer, &diagnostic->message);
   (*diagnostic_starter (context)) (context, diagnostic);
   pp_output_formatted_text (context->printer);
+  diagnostic_show_locus (context, diagnostic);
   (*diagnostic_finalizer (context)) (context, diagnostic);
   pp_flush (context->printer);
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->x_data = NULL;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 186103)
+++ gcc/diagnostic.h	(working copy)
@@ -97,10 +97,17 @@ struct diagnostic_context
 
   /* For pragma push/pop.  */
   int *push_list;
   int n_push;
 
+  /* True if we should print the source line with a caret indicating
+     the location.  */
+  bool show_caret;
+
+  /* Maximum width of the source line printed.  */
+  int caret_max_width;
+
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
   /* True if we should raise a SIGABRT on errors.  */
@@ -252,10 +259,11 @@ extern diagnostic_context *global_dc;
 
 /* Diagnostic related functions.  */
 extern void diagnostic_initialize (diagnostic_context *, int);
 extern void diagnostic_finish (diagnostic_context *);
 extern void diagnostic_report_current_module (diagnostic_context *, location_t);
+extern void diagnostic_show_locus (diagnostic_context *, const diagnostic_info *);
 
 /* Force diagnostics controlled by OPTIDX to be kind KIND.  */
 extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *,
 						    int /* optidx */,
 						    diagnostic_t /* kind */,
@@ -273,10 +281,12 @@ extern void diagnostic_set_info_translat
      ATTRIBUTE_GCC_DIAG(2,0);
 #endif
 extern char *diagnostic_build_prefix (diagnostic_context *, diagnostic_info *);
 void default_diagnostic_starter (diagnostic_context *, diagnostic_info *);
 void default_diagnostic_finalizer (diagnostic_context *, diagnostic_info *);
+void diagnostic_set_caret_max_width (diagnostic_context *context, int value);
+
 
 /* Pure text formatting support functions.  */
 extern char *file_name_as_prefix (const char *);
 
 #endif /* ! GCC_DIAGNOSTIC_H */
Index: gcc/input.c
===================================================================
--- gcc/input.c	(revision 186103)
+++ gcc/input.c	(working copy)
@@ -48,10 +48,69 @@ expand_location (source_location loc)
     xloc.file = loc == UNKNOWN_LOCATION ? NULL : _("<built-in>");
 
   return xloc;
 }
 
+/* Reads one line from file into a static buffer.  */
+static const char *
+read_line (FILE *file)
+{
+  static char *string;
+  static size_t string_len;
+  size_t pos = 0;
+  char *ptr;
+
+  if (!string_len)
+    {
+      string_len = 200;
+      string = XNEWVEC (char, string_len);
+    }
+
+  while ((ptr = fgets (string + pos, string_len - pos, file)))
+    {
+      size_t len = strlen (string + pos);
+
+      if (string[pos + len - 1] == '\n')
+	{
+	  string[pos + len - 1] = 0;
+	  return string;
+	}
+      pos += len;
+      ptr = XNEWVEC (char, string_len * 2);
+      if (ptr)
+	{
+	  memcpy (ptr, string, pos);
+	  string = ptr;
+	  string_len += 2;
+	}
+      else
+	pos = 0;
+    }
+      
+  return pos ? string : NULL;
+}
+
+/* Return the physical source line that corresponds to xloc in a
+   buffer that is statically allocated.  The newline is replaced by
+   the null character.  */
+
+const char *
+location_get_source_line(expanded_location xloc)
+{
+  const char *buffer;
+  int lines = 1;
+  FILE *stream = xloc.file ? fopen (xloc.file, "r") : NULL;
+  if (!stream)
+    return NULL;
+
+  while ((buffer = read_line (stream)) && lines < xloc.line)
+    lines++;
+
+  fclose (stream);
+  return buffer;
+}
+
 #define ONE_K 1024
 #define ONE_M (ONE_K * ONE_K)
 
 /* Display a number as an integer multiple of either:
    - 1024, if said integer is >= to 10 K (in base 2)
Index: gcc/input.h
===================================================================
--- gcc/input.h	(revision 186103)
+++ gcc/input.h	(working copy)
@@ -36,10 +36,11 @@ extern GTY(()) struct line_maps *line_ta
    both UNKNOWN_LOCATION and BUILTINS_LOCATION fit into that.  */
 extern char builtins_location_check[(BUILTINS_LOCATION
 				     < RESERVED_LOCATION_COUNT) ? 1 : -1];
 
 extern expanded_location expand_location (source_location);
+extern const char * location_get_source_line(expanded_location xloc);
 
 /* Historically GCC used location_t, while cpp used source_location.
    This could be removed but it hardly seems worth the effort.  */
 typedef source_location location_t;
 
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c	(revision 186103)
+++ gcc/toplev.c	(working copy)
@@ -1167,10 +1167,12 @@ general_init (const char *argv0)
      finalizer -- for tokens resulting from macro macro expansion.  */
   diagnostic_finalizer (global_dc) = virt_loc_aware_diagnostic_finalizer;
   /* Set a default printer.  Language specific initializations will
      override it later.  */
   pp_format_decoder (global_dc->printer) = &default_tree_printer;
+  global_dc->show_caret
+    = global_options_init.x_flag_diagnostics_show_caret;
   global_dc->show_option_requested
     = global_options_init.x_flag_diagnostics_show_option;
   global_dc->show_column
     = global_options_init.x_flag_show_column;
   global_dc->internal_error = plugins_internal_error_function;
Index: gcc/pretty-print.h
===================================================================
--- gcc/pretty-print.h	(revision 186103)
+++ gcc/pretty-print.h	(working copy)
@@ -199,10 +199,13 @@ struct pretty_print_info
 };
 
 #define pp_set_line_maximum_length(PP, L) \
    pp_base_set_line_maximum_length (pp_base (PP), L)
 #define pp_set_prefix(PP, P)    pp_base_set_prefix (pp_base (PP), P)
+#define pp_get_prefix(PP)       pp_base_get_prefix (pp_base (PP))
+static inline const char *
+pp_base_get_prefix (const pretty_printer *pp) { return pp->prefix; }
 #define pp_destroy_prefix(PP)   pp_base_destroy_prefix (pp_base (PP))
 #define pp_remaining_character_count_for_line(PP) \
   pp_base_remaining_character_count_for_line (pp_base (PP))
 #define pp_clear_output_area(PP) \
   pp_base_clear_output_area (pp_base (PP))
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp	(revision 186103)
+++ gcc/testsuite/lib/prune.exp	(working copy)
@@ -15,10 +15,12 @@
 # along with GCC; see the file COPYING3.  If not see
 # <http://www.gnu.org/licenses/>.
 
 # Prune messages from gcc that aren't useful.
 
+set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS"
+
 proc prune_gcc_output { text } {
     #send_user "Before:$text\n"
 
     regsub -all "(^|\n)(\[^\n\]*: )?In ((static member |lambda )?function|member|method|(copy )?constructor|destructor|instantiation|substitution|program|subroutine|block-data)\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[^\n\]*(: )?At (top level|global scope):\[^\n\]*" $text "" text
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 186103)
+++ gcc/dwarf2out.c	(working copy)
@@ -18367,10 +18367,11 @@ gen_producer_string (void)
       case OPT_grecord_gcc_switches:
       case OPT_gno_record_gcc_switches:
       case OPT__output_pch_:
       case OPT_fdiagnostics_show_location_:
       case OPT_fdiagnostics_show_option:
+      case OPT_fdiagnostics_show_caret:
       case OPT_fverbose_asm:
       case OPT____:
       case OPT__sysroot_:
       case OPT_nostdinc:
       case OPT_nostdinc__:
Index: gcc/opts.c
===================================================================
--- gcc/opts.c	(revision 186103)
+++ gcc/opts.c	(working copy)
@@ -1497,10 +1497,14 @@ common_handle_option (struct gcc_options
       break;
 
     case OPT_fdiagnostics_show_location_:
       diagnostic_prefixing_rule (dc) = (diagnostic_prefixing_rule_t) value;
       break;
+ 
+    case OPT_fdiagnostics_show_caret:
+      dc->show_caret = value;
+      break;
 
     case OPT_fdiagnostics_show_option:
       dc->show_option_requested = value;
       break;
 
@@ -1537,10 +1541,11 @@ common_handle_option (struct gcc_options
 	(&opts->x_flag_instrument_functions_exclude_files, arg);
       break;
 
     case OPT_fmessage_length_:
       pp_set_line_maximum_length (dc->printer, value);
+      diagnostic_set_caret_max_width (dc, value);
       break;
 
     case OPT_fpack_struct_:
       if (value <= 0 || (value & (value - 1)) || value > 16)
 	error_at (loc,
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 186103)
+++ gcc/common.opt	(working copy)
@@ -997,10 +997,14 @@ EnumValue
 Enum(diagnostic_prefixing_rule) String(once) Value(DIAGNOSTICS_SHOW_PREFIX_ONCE)
 
 EnumValue
 Enum(diagnostic_prefixing_rule) String(every-line) Value(DIAGNOSTICS_SHOW_PREFIX_EVERY_LINE)
 
+fdiagnostics-show-caret
+Common Var(flag_diagnostics_show_caret) Init(1)
+Show the source line with a caret indicating the column
+
 fdiagnostics-show-option
 Common Var(flag_diagnostics_show_option) Init(1)
 Amend appropriate diagnostic messages with the command line option that controls them
 
 fdisable-

Re: [PATCH] Caret diagnostics

[Mike Stump]

On Apr 10, 2012, at 11:52 AM, Gabriel Dos Reis wrote:
> On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
> <lopezibanez@gmail.com> wrote:
>> On 10 April 2012 00:28, Jason Merrill <jason@redhat.com> wrote:
>>> On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
>>>> 
>>>> * It uses  the default cutoff as max_width, whatever it is (as
>>>> controlled by -fmessage-length).
>>>> * It uses the pretty-printer. The text cannot (should not) wrap
>>>> because we still print only max_width chars at most.
>>> 
>>> 
>>> Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
>>> to use COLUMNS to limit how much of the source we print.
>> 
>> Like this?
> 
> There is a novelty in this new version that I don't think we discussed
> before: automatic expansion of tabs to 8 hard space characters.  That
> number should not be hardcoded as there is no reason to believe a tab
> character always expands to 8 space characters.  You should check
> the environment first; if not present the default expansion number should
> be a symbolic constant as opposed to a magic number sprinkled all over the
> places.  I would also encourage you to introduce more abstraction to
> reduce the size of diagnostic_show_locus.

Knobs have a cost.  Having 1000s of command line options doesn't really make a product better.  Our job is to provide what we must and omit what we can.  You argue for a knob that no user has asked for or expressed a need for.  I think we should have a much higher bar than this.  The reality is, tabs are 8, period.  There was a time went editors managed tabs stops by changing column into which a tab would move, this ability taken from devices called typewriters which had a little metal bit that you moved around where you wanted tab stops.  The days of there devices called typewriters are over, we no longer cater to them.  Today, editors can manage what the key called TAB does, and its semantics are disconnected from the file save format.  This file format can be set at 8, while the edit function is indent 2, 4, 8 or as complex as what emacs does with c-mode.  It is a bad idea to have a file format tab is other than 8, and we should encourage all but legacy people to convert to 8.  We do this by having a wiki entry and explains the use of pr, and how to set the file save format to 8, if we need to.  For legacy people, they don't _want_ a new compiler, pretty much by definition.  So, I'd argue for a symbol constant, but, a constant nonetheless.  If a large user _had_ serious needs for something other than 8, _they_ can change the compiler to:

  #define TAB_COLS (getenv("TABS") ? atoi (genenv("TABS")) : 8)

if they want.  I don't think we should have a knob for this.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Tue, Apr 10, 2012 at 5:23 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Apr 10, 2012, at 11:52 AM, Gabriel Dos Reis wrote:
>> On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
>> <lopezibanez@gmail.com> wrote:
>>> On 10 April 2012 00:28, Jason Merrill <jason@redhat.com> wrote:
>>>> On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
>>>>>
>>>>> * It uses  the default cutoff as max_width, whatever it is (as
>>>>> controlled by -fmessage-length).
>>>>> * It uses the pretty-printer. The text cannot (should not) wrap
>>>>> because we still print only max_width chars at most.
>>>>
>>>>
>>>> Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
>>>> to use COLUMNS to limit how much of the source we print.
>>>
>>> Like this?
>>
>> There is a novelty in this new version that I don't think we discussed
>> before: automatic expansion of tabs to 8 hard space characters.  That
>> number should not be hardcoded as there is no reason to believe a tab
>> character always expands to 8 space characters.  You should check
>> the environment first; if not present the default expansion number should
>> be a symbolic constant as opposed to a magic number sprinkled all over the
>> places.  I would also encourage you to introduce more abstraction to
>> reduce the size of diagnostic_show_locus.
>
> Knobs have a cost.  Having 1000s of command line options doesn't really make a product better.  Our job is to provide what we must and omit what we can.  You argue for a knob that no user has asked for or expressed a need for.  I think we should have a much higher bar than this.  The reality is, tabs are 8, period.  There was a time went editors managed tabs stops by changing column into which a tab would move, this ability taken from devices called typewriters which had a little metal bit that you moved around where you wanted tab stops.  The days of there devices called typewriters are over, we no longer cater to them.  Today, editors can manage what the key called TAB does, and its semantics are disconnected from the file save format.  This file format can be set at 8, while the edit function is indent 2, 4, 8 or as complex as what emacs does with c-mode.  It is a bad idea to have a file format tab is other than 8, and we should encourage all but legacy people to convert to 8.  We do this by having a wiki entry and explains the use of pr, and how to set the file save format to 8, if we need to.  For legacy people, they don't _want_ a new compiler, pretty much by definition.  So, I'd argue for a symbol constant, but, a constant nonetheless.  If a large user _had_ serious needs for something other than 8, _they_ can change the compiler to:
>
>  #define TAB_COLS (getenv("TABS") ? atoi (genenv("TABS")) : 8)
>
> if they want.  I don't think we should have a knob for this.

This is fully mystifying to me and I still do not understand what it
is all about after reading it 8 times.

- Gaby

Re: [PATCH] Caret diagnostics

[Mike Stump]

On Apr 10, 2012, at 4:37 PM, Gabriel Dos Reis wrote:
>>> You should check the environment first


>> I don't think we should have a knob for this.

> This is fully mystifying to me and I still do not understand what it is all about after reading it 8 times.

Let me rephrase, I disagree, we should not check the environment first.

I assume, you meant checking the environment, git getenv, for some named variable to control the expansion of tabs.  Yes?

Re: [PATCH] Caret diagnostics

[Jason Merrill]

On 04/10/2012 04:33 PM, Manuel López-Ibáñez wrote:
> A new version of the patch. It removes the special handling of tabs.
> And abstracts out the adjusting of the line as Gabriel suggested. I am
> not sure what else could be abstracted out. The function is much
> shorter than other functions in diagnostic.c.
>
> This patch builds and I am running a full boostrap+check now.
>
> OK if it passes?

OK.

Jason

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Tue, Apr 10, 2012 at 7:54 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Apr 10, 2012, at 4:37 PM, Gabriel Dos Reis wrote:
>>>> You should check the environment first
>
>
>>> I don't think we should have a knob for this.
>
>> This is fully mystifying to me and I still do not understand what it is all about after reading it 8 times.
>
> Let me rephrase, I disagree, we should not check the environment first.
>
> I assume, you meant checking the environment, git getenv, for some named variable to control the expansion of tabs.  Yes?

Yes, because tabs expansion is very much environment dependent.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Tue, Apr 10, 2012 at 2:37 PM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 10 April 2012 20:52, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
>> On Tue, Apr 10, 2012 at 11:46 AM, Manuel López-Ibáñez
>> <lopezibanez@gmail.com> wrote:
>>> On 10 April 2012 00:28, Jason Merrill <jason@redhat.com> wrote:
>>>> On 04/09/2012 04:01 PM, Manuel López-Ibáńez wrote:
>>>>>
>>>>> * It uses  the default cutoff as max_width, whatever it is (as
>>>>> controlled by -fmessage-length).
>>>>> * It uses the pretty-printer. The text cannot (should not) wrap
>>>>> because we still print only max_width chars at most.
>>>>
>>>>
>>>> Hmm, I think if pp_line_cutoff is 0 and we're on a terminal, we still want
>>>> to use COLUMNS to limit how much of the source we print.
>>>
>>> Like this?
>>
>> There is a novelty in this new version that I don't think we discussed
>> before: automatic expansion of tabs to 8 hard space characters.  That
>> number should not be hardcoded as there is no reason to believe a tab
>> character always expands to 8 space characters.  You should check
>> the environment first; if not present the default expansion number should
>> be a symbolic constant as opposed to a magic number sprinkled all over the
>> places.  I would also encourage you to introduce more abstraction to
>> reduce the size of diagnostic_show_locus.
>
> There is no novelty, it has been there from the beginning, and now I
> realize it was a mistake to do the extra work to implement this.

I wonder how I missed it in the previous reviews.

>
> The GCS says: "Calculate column numbers assuming that space and all
> ASCII printing characters have equal width, and assuming tab stops
> every 8 columns"
> http://www.gnu.org/prep/standards/standards.html#Errors

That is GNU coding convention for GNU codes.  We do not require that
programs GCC accept have to adhere to GNU coding convention, not
should we.  An important aspect of diagnostic with caret is that we are
writing back to the user what she wrote, not what GNU wanted her
to write.

> So, in fact, libcpp is buggy for not implementing this (as can be seen
> in emacs). If/When libcpp is fixed, the column info will be correct
> for tabs. And then, one may care about printing tabs as anything
> different from a single space. But until then, a single space is a
> good as anything. In fact this is what is done in c-ppoutput.c.
>

I would not necessarily say that libcpp is buggy in this aspect.
However, I do agree that removing the tab expansion simplifies
the code and is better.

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Tue, Apr 10, 2012 at 3:33 PM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 10 April 2012 21:41, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
>> On 10 April 2012 21:31, Jason Merrill <jason@redhat.com> wrote:
>>> On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:
>>>>
>>>> +  max_width = context->caret_max_width;
>>>> +  if (max_width<= 0)
>>>> +    max_width = INT_MAX;
>>>
>>>
>>> I don't think we need the test here; diagnostic_set_caret_max_width should
>>> make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.
>>
>> It was just a bit of defensive programming, since nothing stops anyone
>> to set the value directly. But I will remove it.
>>
>>> On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:
>>>>
>>>> There is a novelty in this new version that I don't think we discussed
>>>> before: automatic expansion of tabs to 8 hard space characters.  That
>>>> number should not be hardcoded as there is no reason to believe a tab
>>>> character always expands to 8 space characters.  You should check
>>>> the environment first; if not present the default expansion number should
>>>> be a symbolic constant as opposed to a magic number sprinkled all over the
>>>> places.
>>>
>>>
>>> Hmm.  I don't know if there's any reliable way to query tab stops; the "it"
>>> termcap/terminfo capability tells you what it was originally (presumably 8),
>>> but it might have changed.
>>
>> No idea either, but it is in fact easier to print tabs a single
>> spaces. This simplifies the code a lot, as pointed by Gabriel. So if
>> you also prefer the simpler version, I am fine with committing that
>> one (it also saves space in the output).
>
> A new version of the patch. It removes the special handling of tabs.
> And abstracts out the adjusting of the line as Gabriel suggested. I am
> not sure what else could be abstracted out. The function is much
> shorter than other functions in diagnostic.c.
>
> This patch builds and I am running a full boostrap+check now.
>
> OK if it passes?

OK.

Re: [PATCH] Caret diagnostics

[H.J. Lu]

On Tue, Apr 10, 2012 at 1:33 PM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> On 10 April 2012 21:41, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
>> On 10 April 2012 21:31, Jason Merrill <jason@redhat.com> wrote:
>>> On 04/10/2012 12:46 PM, Manuel López-Ibáñez wrote:
>>>>
>>>> +  max_width = context->caret_max_width;
>>>> +  if (max_width<= 0)
>>>> +    max_width = INT_MAX;
>>>
>>>
>>> I don't think we need the test here; diagnostic_set_caret_max_width should
>>> make sure caret_max_width is set sensibly.  Otherwise, yes, thanks.
>>
>> It was just a bit of defensive programming, since nothing stops anyone
>> to set the value directly. But I will remove it.
>>
>>> On 04/10/2012 02:52 PM, Gabriel Dos Reis wrote:
>>>>
>>>> There is a novelty in this new version that I don't think we discussed
>>>> before: automatic expansion of tabs to 8 hard space characters.  That
>>>> number should not be hardcoded as there is no reason to believe a tab
>>>> character always expands to 8 space characters.  You should check
>>>> the environment first; if not present the default expansion number should
>>>> be a symbolic constant as opposed to a magic number sprinkled all over the
>>>> places.
>>>
>>>
>>> Hmm.  I don't know if there's any reliable way to query tab stops; the "it"
>>> termcap/terminfo capability tells you what it was originally (presumably 8),
>>> but it might have changed.
>>
>> No idea either, but it is in fact easier to print tabs a single
>> spaces. This simplifies the code a lot, as pointed by Gabriel. So if
>> you also prefer the simpler version, I am fine with committing that
>> one (it also saves space in the output).
>
> A new version of the patch. It removes the special handling of tabs.
> And abstracts out the adjusting of the line as Gabriel suggested. I am
> not sure what else could be abstracted out. The function is much
> shorter than other functions in diagnostic.c.
>
> This patch builds and I am running a full boostrap+check now.
>
> OK if it passes?
>
> Cheers,
>
> Manuel.
>
>
> 2012-04-05  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>
>        PR 24985
> libstdc++-v3/
>        * testsuite/lib/prune.exp: Handle caret.
> libmudflap/
>        * testsuite/lib/libmudflap.exp: Handle caret.
> gcc/
>        * diagnostic.h (show_caret): Declare.
>        (caret_max_width): Declare.
>        (diagnostic_show_locus): Declare.
>        * diagnostic.c (diagnostic_initialize): Initialize to false.
>        (diagnostic_show_locus): New.
>        (diagnostic_report_diagnostic): Call it.
>        (getenv_columns): New.
>        (adjust_line): New.
>        (diagnostic_set_caret_max_width): New.
>        * input.c (read_line): New.
>        (location_get_source_line): New.
>        * input.h (location_get_source_line): Declare.
>        * toplev.c (general_init): Initialize show_caret from options.
>        * dwarf2out.c (gen_producer_string): Handle fdiagnostics-show-caret.
>        * opts.c (common_handle_option): Likewise.
>        * pretty-print.h (pp_get_prefix): New.
>        (pp_base_get_prefix): New.
>        * common.opt (fdiagnostics-show-caret): New option.
>        * doc/invoke.texi (fdiagnostics-show-caret): Document it.
> testsuite/
>        * lib/prune.exp: Add -fno-diagnostics-show-caret.

It breaks library tests:

ERROR: tcl error sourcing
../../../../src-trunk/boehm-gc/testsuite/../../gcc/testsuite/lib/prune.exp.
ERROR: tcl error sourcing
../../../../src-trunk/libgomp/testsuite/../../gcc/testsuite/lib/prune.exp.
ERROR: tcl error sourcing
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.

RROR: tcl error sourcing
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.
can't read "TEST_ALWAYS_FLAGS": no such variable
    while executing
"set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS""
    (file "../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp"
line 20)
    invoked from within
"source ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source
../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp"
    invoked from within
"catch "uplevel #0 source $file""
make[8]: *** [check-DEJAGNU] Error 1

-- 
H.J.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

In 11 April 2012 18:40, H.J. Lu <hjl.tools@gmail.com> wrote:
>
> It breaks library tests:
>
> ERROR: tcl error sourcing
> ../../../../src-trunk/boehm-gc/testsuite/../../gcc/testsuite/lib/prune.exp.
> ERROR: tcl error sourcing
> ../../../../src-trunk/libgomp/testsuite/../../gcc/testsuite/lib/prune.exp.
> ERROR: tcl error sourcing
> ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.
>
> RROR: tcl error sourcing
> ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.
> can't read "TEST_ALWAYS_FLAGS": no such variable
>    while executing
> "set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS""
>    (file "../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp"
> line 20)
>    invoked from within
> "source ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp"
>    ("uplevel" body line 1)
>    invoked from within
> "uplevel #0 source
> ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp"
>    invoked from within
> "catch "uplevel #0 source $file""
> make[8]: *** [check-DEJAGNU] Error 1

contrib/compare_tests does not seem to detect this :-(

So how can I say in tcl "if not defined VAR then set VAR ""?

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Andrew Pinski]

On Wed, Apr 11, 2012 at 11:15 AM, Manuel López-Ibáñez
<lopezibanez@gmail.com> wrote:
> So how can I say in tcl "if not defined VAR then set VAR ""?

if ![info exists VAR] {
  set VAR XXX
}

Thanks,
Andrew Pinski

Re: [PATCH] Caret diagnostics

[Mike Stump]

On Apr 11, 2012, at 11:15 AM, Manuel López-Ibáñez wrote:
> contrib/compare_tests does not seem to detect this :-(

I'd bet, either, there were no tests after that point, or, compare_tests pointed it out.  Feel free to send the two .sum files and and I will investigate if you think that isn't the case.

Re: [PATCH] Caret diagnostics

[Manuel López-Ibáñez]

On 11 April 2012 18:40, H.J. Lu <hjl.tools@gmail.com> wrote:
>
> It breaks library tests:
>
> ERROR: tcl error sourcing
> ../../../../src-trunk/boehm-gc/testsuite/../../gcc/testsuite/lib/prune.exp.
> ERROR: tcl error sourcing
> ../../../../src-trunk/libgomp/testsuite/../../gcc/testsuite/lib/prune.exp.
> ERROR: tcl error sourcing
>

Hopefully fixed by this patch:

Index: libgomp/ChangeLog
===================================================================
--- libgomp/ChangeLog   (revision 186352)
+++ libgomp/ChangeLog   (revision 186353)
@@ -1,3 +1,7 @@
+2012-04-11  Manuel López-Ibáñez  <manu@gcc.gnu.org>
+
+       * testsuite/lib/libgomp.exp: Add -fno-diagnostics-show-caret.
+
 2012-03-31  H.J. Lu  <hongjiu.lu@intel.com>

        PR bootstrap/52812
Index: libgomp/testsuite/lib/libgomp.exp
===================================================================
--- libgomp/testsuite/lib/libgomp.exp   (revision 186352)
+++ libgomp/testsuite/lib/libgomp.exp   (revision 186353)
@@ -161,6 +161,9 @@
     # error-message parsing machinery.
     lappend ALWAYS_CFLAGS "additional_flags=-fmessage-length=0"

+    # Disable caret
+    lappend ALWAYS_CFLAGS "additional_flags=-fno-diagnostics-show-caret"
+
     # And, gee, turn on OpenMP.
     lappend ALWAYS_CFLAGS "additional_flags=-fopenmp"
 }
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp (revision 186352)
+++ gcc/testsuite/lib/prune.exp (revision 186353)
@@ -17,6 +17,9 @@

 # Prune messages from gcc that aren't useful.

+if ![info exists TEST_ALWAYS_FLAGS] {
+    set TEST_ALWAYS_FLAGS ""
+}
 set TEST_ALWAYS_FLAGS "-fno-diagnostics-show-caret $TEST_ALWAYS_FLAGS"

 proc prune_gcc_output { text } {
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog     (revision 186352)
+++ gcc/testsuite/ChangeLog     (revision 186353)
@@ -1,3 +1,7 @@
+2012-04-11  Manuel López-Ibáñez  <manu@gcc.gnu.org>
+
+       * lib/prune.exp (TEST_ALWAYS_FLAGS): If undefined, set to empty.
+
 ../../../../src-trunk/libitm/testsuite/../../gcc/testsuite/lib/prune.exp.


Committed after testing as revision 186353.

Cheers,

Manuel.

Re: [PATCH] Caret diagnostics

[Tom Tromey]

>>>>> "Gaby" == Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

Manuel> So, in fact, libcpp is buggy for not implementing this (as can be seen
Manuel> in emacs). If/When libcpp is fixed, the column info will be correct
Manuel> for tabs. And then, one may care about printing tabs as anything
Manuel> different from a single space. But until then, a single space is a
Manuel> good as anything. In fact this is what is done in c-ppoutput.c.

Gaby> I would not necessarily say that libcpp is buggy in this aspect.
Gaby> However, I do agree that removing the tab expansion simplifies
Gaby> the code and is better.

Unfortunately I think it really is buggy.  Other GNU programs, including
Emacs, already conform to the GCS here.  Arguably the GCS made the wrong
decision, but it is already baked in all over.

Tom

Re: [PATCH] Caret diagnostics

[Gabriel Dos Reis]

On Thu, Apr 12, 2012 at 11:53 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Gaby" == Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>
> Manuel> So, in fact, libcpp is buggy for not implementing this (as can be seen
> Manuel> in emacs). If/When libcpp is fixed, the column info will be correct
> Manuel> for tabs. And then, one may care about printing tabs as anything
> Manuel> different from a single space. But until then, a single space is a
> Manuel> good as anything. In fact this is what is done in c-ppoutput.c.
>
> Gaby> I would not necessarily say that libcpp is buggy in this aspect.
> Gaby> However, I do agree that removing the tab expansion simplifies
> Gaby> the code and is better.
>
> Unfortunately I think it really is buggy.  Other GNU programs, including
> Emacs, already conform to the GCS here.  Arguably the GCS made the wrong
> decision, but it is already baked in all over.

OK.

-- Gaby

Re: [PATCH] caret diagnostics

[Paolo Bonzini]

> I don't know why it was considered bad (and had thought it was simply 
> never reviewed), but I am happy to review such patches as i18n maintainer 
> (including in Stage 3 - they are clearly bug fixes) if the front-end 
> maintainers don't reject them.  However, I think they would need splitting 
> up for review.

Yes, of course.  Unfortunately I don't have much time for submitting 
patches at all... :-(  I'll take note though of your kind offer to 
review this kind of patch.

> I'm not convinced by the cp-i18n.c approach, however, although there may 
> be cases where it's necessary. 

I see.  In fact, the main problem was about the cp-i18n.c file in the 
old review (I *think* by Geoff Keating, but I'm not sure).

> My preference would be to use full 
> sentences if at all possible (see the WARN_FOR_ASSIGNMENT macro in the C 
> front end and other diagnostics in convert_for_assignment, for example, or 
> readonly_error for another such case).

Yes, I know about them.  I would also prefer to have full sentences when 
possible.

Paolo

Re: [PATCH] caret diagnostics

[Joseph S. Myers]

On Sat, 16 Aug 2008, Paolo Bonzini wrote:

> Chris Lattner wrote:
> > Please don't forget C++.
> 
> C++ is a completely lost battle.  I had a patch to handle all the %s in the
> source base, but my approach (which basically implied adding translatable
> strings for "a class", "to a class", "with a class", "in a class", etc. to
> account for languages with cases) was considered bad.
> 
> See http://gcc.gnu.org/ml/gcc-patches/2002-06/msg01620.html for my patch.  I
> cannot find the answer.

I don't know why it was considered bad (and had thought it was simply 
never reviewed), but I am happy to review such patches as i18n maintainer 
(including in Stage 3 - they are clearly bug fixes) if the front-end 
maintainers don't reject them.  However, I think they would need splitting 
up for review.  For example:

* Local changes that simply use two or more sentences in a particular 
place should be separate from more complicated changes like the cp-i18n.c 
there (but many such local changes could be grouped together).

* If passing around an enumeration rather than a sentence fragment, such 
changes should be split up according to the function whose interface they 
change.

* Where there are relevant bug reports (e.g. 29017 29897 29917 31665 
34836) please make sure to include them in the ChangeLog message.

* I am not a C++ expert, so if the patch depends on C++ details for which 
combinations of cases can actually occur and so need messages included 
then it may still need a C++ maintainer review.

I'm not convinced by the cp-i18n.c approach, however, although there may 
be cases where it's necessary.  My preference would be to use full 
sentences if at all possible (see the WARN_FOR_ASSIGNMENT macro in the C 
front end and other diagnostics in convert_for_assignment, for example, or 
readonly_error for another such case).  In some cases where this isn't 
feasible in the source (c_parse_error, format checking - I think all other 
C cases are fixed) it might be possible for the exgettext script to create 
all combinations and for full sentences still to be passed for translation

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics

[Paolo Bonzini]

Chris Lattner wrote:
> 
> On Aug 14, 2008, at 8:47 AM, Joseph S. Myers wrote:
> 
>> On Thu, 14 Aug 2008, Robert Dewar wrote:
>>
>>> BTW, I am all in favor of caret output, it's not the default in
>>> gnat, the default is more like the C default, but -gnatv gives
>>> output like:
>>
>> And I'd hope we could keep things that way for both C and Ada, but make
> 
> Please don't forget C++.

C++ is a completely lost battle.  I had a patch to handle all the %s in 
the source base, but my approach (which basically implied adding 
translatable strings for "a class", "to a class", "with a class", "in a 
class", etc. to account for languages with cases) was considered bad.

See http://gcc.gnu.org/ml/gcc-patches/2002-06/msg01620.html for my 
patch.  I cannot find the answer.

Paolo

Re: [PATCH] caret diagnostics

[Robert Dewar]

Gabriel Dos Reis wrote:
> I'm in favor of getting -fdiagnostics-show-caret=no by default in this release,
> and enable people like you to get useful stuff done.  That gives us time
> to iron out outstanding bugs for the next release (and making it the default).

That's a good idea regardless of the eventual decision on
making it the default.

Re: [PATCH] caret diagnostics

[Robert Dewar]

Gabriel Dos Reis wrote:
> On Thu, Aug 14, 2008 at 11:52 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Joseph" == Joseph S Myers <joseph@codesourcery.com> writes:
> 
>> I'd like to see carets on by default as part of a major release -- say
>> GCC 5.0.  (First mention!!)
> 
> 100% agreed.
> 
> -- Gaby

As I have mentioned before, we have always had very accurate caret
diagnostics (though we use | instead of ^ :-)) in GNAT, but decided
that brief error messages were the default. 97% of the time,
especially for experienced programmers, you really don't need
this information, and it is not worth the extra complexity in
the output. Furthermore, you certainly don't want this is you
are using an IDE which interprets the column numbers in the brief
messages and jumps you to the right location anyway (the great
majority of Ada users are using an IDE, not all, but certainly
most). That's not to say some of our users do not find the
caret diagnostics very valuable, and of course thats reasonable
as an option, but I wouldn't make it the default.

Now one significant difference between Ada and C in the gcc
arena is that the messages by default from GNAT always include
(highly accurate) column numbers, and that seems desirable
anyway, but assuming (I hope I assume right) that the intent
of caret diagnostics is to actually post flags in the columns,
I don't think that should be the default.

One argument in favor of making this the default is that this
is the only way to smoke out errors, but I think you could do
just as good a job of this by always displaying column numbers
and anyway using an IDE, including EMACS (I assume emacs can
handle columns fine), will notice problems and can be encouraged
to report problems once the messages get good enough to be
reasonable.

Another interesting capability in GNAT is the -gnatjnn flag which
causes a message and its continuation lines to be wrapped together
as a single message, and then broken into lines nn long:

Consider

      1. procedure q is
      2.    type u8 is range 1 .. 10;
      3.    a,b : u8 := 10;
      4. begin
      5.    a := b + 1;
      6. end;

default mode

q.adb:5:11: warning: value not in range of type "u8" defined at line 2
q.adb:5:11: warning: "Constraint_Error" will be raised at run time

in -gnatv mode:

      5.    a := b + 1;
                   |
         >>> warning: value not in range of type "u8" defined at line 2
         >>> warning: "Constraint_Error" will be raised at run time

in -gnatl mode:

      1. procedure q is
      2.    type u8 is range 1 .. 10;
      3.    a,b : u8 := 10;
      4. begin
      5.    a := b + 1;
                   |
         >>> warning: value not in range of type "u8" defined at line 2
         >>> warning: "Constraint_Error" will be raised at run time

      6. end;

default with -gnatj60

  q.adb:5:11: warning: value not in range of type "u8"
              defined at line 2, "Constraint_Error" will be
              raised at run time

-gnatv with -gnatj60

      5.    a := b + 1;
                   |
         >>> warning: value not in range of type "u8"
             defined at line 2, "Constraint_Error" will be
             raised at run time

-gnatl with -gnatj60

      1. procedure q is
      2.    type u8 is range 1 .. 10;
      3.    a,b : u8 := 10;
      4. begin
      5.    a := b + 1;
                   |
         >>> warning: value not in range of type "u8"
             defined at line 2, "Constraint_Error" will be
             raised at run time

      6. end;

Another possibility is -gnatjn999 which gathers a message
and all its continuation messages in one line, which
may be useful in an IDE context where you would like
the IDE to format the message (e.g. in a popup box)

The one thing that GNAT does not have is a notion of ranges
(in the above it would be a little clearer perhaps if the
range b+1 were flagged, but in fact given that we are
pointing to an overflow, pointing to the operator causing
it is worthwhile.

Re: [PATCH] caret diagnostics

[Gabriel Dos Reis]

On Thu, Aug 14, 2008 at 11:52 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Joseph" == Joseph S Myers <joseph@codesourcery.com> writes:

> I'd like to see carets on by default as part of a major release -- say
> GCC 5.0.  (First mention!!)

100% agreed.

-- Gaby

Re: [PATCH] caret diagnostics

[Mark Mitchell]

Manuel López-Ibáñez wrote:

> The locations are as wrong now as they will be after enabling caret
> diagnostics. The only difference is that no one pays attention right
> now because that would require to enable -fshow-column and count
> column numbers. So I am certainly not "breaking the compiler and
> hoping that other developers will fix it". 

OK, instead of "break", I will say that you are hoping to change the 
user-visible behavior in a way that makes a current limitation obvious, 
in the hope that will prod people to fix it.  I don't think that's 
appropriate.

Having cc1 print a statistic (when not passed "-quiet") saying "I got 
error messages wrong 100 times in this translation unit" would be fine 
(if it were possible to compute) since it wouldn't get in anyone's way. 
  Your change will inconvenience people.

Please attack the problem directly, by working with others to improve 
the location information in the compiler.  My experience has been that 
if you get the ball rolling, other people will get behind and help, when 
they see that the problem is tractable.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] caret diagnostics

[Manuel López-Ibáñez]

2008/8/14 Mark Mitchell <mark@codesourcery.com>:
> Manuel López-Ibáñez wrote:
>
>> My proposal is to enable this while not in release mode (like we
>> enable checks and dump statistics and output from the optimizers), so
>> developers notice wrong locations and open a PR and maybe even fix
>> them.
>
> Manuel --
>
> We can all work together, but the approach of "break the compiler and hope
> that other developers will fix it" is not a good plan.  As Joseph says, this
> needs to be configured off by default (for compatibility with the way GCC
> has worked since forever, and for compatibility with the GNU standards), and
> there needs to be an option to turn it on.  You will need to recruit others
> who are willing to invest in helping to improve the new mode's quality.

The locations are as wrong now as they will be after enabling caret
diagnostics. The only difference is that no one pays attention right
now because that would require to enable -fshow-column and count
column numbers. So I am certainly not "breaking the compiler and
hoping that other developers will fix it". This is *exactly* like
printing statistics at the end of the compilation. It may hint you
that something is broken somewhere but you can freely ignore it as a
visual hassle of the experimental compiler.

BTW, all those time/memory statistics didn't break anything when they
were added? And they are given even if no error/warning is printed.

Cheers,

Manuel.

Re: [PATCH] caret diagnostics

[Robert Dewar]

Ralf Wildenhues wrote:
> * Tom Tromey wrote on Thu, Aug 14, 2008 at 07:39:57PM CEST:
>> FWIW -- the gcc-output-parsing tools I care most about are actually
>> usually parsing the output of 'make', which is already full of random
>> undigestible text that must be ignored.  Caret diagnostics are
>> extremely unlike to break these tools.
> 
> But there are editors which are able to parse caret diagnostics from
> compilers.  It's quite helpful for them if caret diagnostic output is
> consistent among compiler/compiler-like tools.  I care about editors :)

Makes more sense for editors to parse column numbers, which they already
do just fine, seems pointless for them to mess with carets.
> 
> Cheers,
> Ralf

Re: [PATCH] caret diagnostics

[Mark Mitchell]

Joseph S. Myers wrote:
> On Thu, 14 Aug 2008, Tom Tromey wrote:
> 
>> Yes, I agree, we need multiple things: accurate locations from the
>> front ends (ideally macro-expansion-aware), start- and end-locations,
>> better diagnostic output of various kinds, perhaps smarter location
>> handling in the optimizations, and of course finally column output in
>> dwarf.  I hope that covers the 80% of stuff we all seem to agree on.
> 
> Regarding the list of improvements, I think most of the benefits from 
> caret diagnostics can be gained simply by printing accurate texts of 
> expressions (or declarations etc.) within the existing one-line diagnostic 
> messages, without needing major and incompatible stylistic changes.

Indeed.  The important thing is not the caret per se; it's having 
accurate line/column information for diagnostics and using the text the 
user wrote, rather than trying to reconstruct expressions.  The caret 
output format may be desirable to some users as well, but most of the 
work here will be in getting the locations correct and in showing the 
user's original text.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] caret diagnostics

[Mark Mitchell]

Manuel López-Ibáñez wrote:

> My proposal is to enable this while not in release mode (like we
> enable checks and dump statistics and output from the optimizers), so
> developers notice wrong locations and open a PR and maybe even fix
> them.

Manuel --

I think it's great that you're working on caret diagnostics.  However, I 
agree 100% with Joseph and Robert.  Users, including other GCC 
developers, are not beta testers.

We can all work together, but the approach of "break the compiler and 
hope that other developers will fix it" is not a good plan.  As Joseph 
says, this needs to be configured off by default (for compatibility with 
the way GCC has worked since forever, and for compatibility with the GNU 
standards), and there needs to be an option to turn it on.  You will 
need to recruit others who are willing to invest in helping to improve 
the new mode's quality.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] caret diagnostics

[Ralf Wildenhues]

* Tom Tromey wrote on Thu, Aug 14, 2008 at 07:39:57PM CEST:
> 
> FWIW -- the gcc-output-parsing tools I care most about are actually
> usually parsing the output of 'make', which is already full of random
> undigestible text that must be ignored.  Caret diagnostics are
> extremely unlike to break these tools.

But there are editors which are able to parse caret diagnostics from
compilers.  It's quite helpful for them if caret diagnostic output is
consistent among compiler/compiler-like tools.  I care about editors :)

Cheers,
Ralf

Re: [PATCH] caret diagnostics

[Tom Tromey]

>>>>> "Ralf" == Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

Ralf> If the GCS-style is not powerful enough to meet GCC's needs, then please
Ralf> let's not only improve GCC, but the GCS document also, so that other
Ralf> programs improve compatibly.

Yeah, good idea.

FWIW -- the gcc-output-parsing tools I care most about are actually
usually parsing the output of 'make', which is already full of random
undigestible text that must be ignored.  Caret diagnostics are
extremely unlike to break these tools.

Tom

Re: [PATCH] caret diagnostics

[Tom Tromey]

>>>>> "Manuel" == Manuel López-Ibáñez <lopezibanez@gmail.com> writes:

Manuel> The approach followed is to never free the input buffers and keep
Manuel> pointers to the start of each line tracked by each line-map.

This seems reasonable, definitely so for a first patch.

Manuel> b) Re-open the file and fseek but that is not trivial since we need to
Manuel> do it fast but still do all character conversions that we did when
Manuel> libcpp opened it the first time.

One argument in favor of this approach is that in a given compilation
we don't emit diagnostics for most source files.  So, why pay the
price of keeping those files in memory?

There are various tradeoffs we could play with.  Like, if we commonly
do not apply character set conversions, we could just mmap the files
and leave them open.  There's a lot of details here I don't remember
offhand, though (like, what kinds of weird stuff does libcpp do with
buffers?).

Manuel> Index: gcc/java/jcf-parse.c
[...]
Manuel> +      /* FIXME CARET: We should add a pointer to the input line
Manuel> +	 instead of NULL.  */

FWIW for gcj, carets don't really make sense, since we are only
compiling class files.  We rely on ecj to emit the nice errors :)

Manuel> +#ifdef USE_CARET_DIAGNOSTICS
Manuel> +      xloc.linebuf = NULL;
Manuel> +#endif

I'm with Joseph on this one -- just add the code, don't make it
conditional.  Mapped locations are not a good historical model to
follow.

Manuel> +  while (max_width > 0 && *line != '\0' && *line != '\n')
Manuel> +    {
Manuel> +      max_width--;
Manuel> +      putchar (*line);
Manuel> +      line++;
Manuel> +    }
Manuel> +  putchar('\n');
Manuel> +  gcc_assert (s.column > 0);
Manuel> +  printf (" %*c\n", s.column, '^');

I think this will run into two problems.

First, the libcpp buffer will be in UTF-8 (or UTF-EBCDIC -- nice), but
the user might be using a different charset.  So, I think we have to
iconv here.

Also, my recollection is that libcpp encodes the column by counting
characters, not by counting columns-as-would-be-displayed.  So, this
code has to have some special handling for TAB characters.

Manuel> Index: gcc/c-tree.h

This seems to be an unrelated change.

I'm not super fond of the inline function SOURCE_LINEBUFFER.  All
those caps read strangely.  And, is this really performance-sensitive
enough to need to be inline?

Unfortunately, libcpp still uses its own diagnostic output code, so
either we'd need to update that as well, or we'd have to finally make
libcpp be able to use the GCC machinery.

Tom

Re: [PATCH] caret diagnostics

[Joseph S. Myers]

On Thu, 14 Aug 2008, Tom Tromey wrote:

> Yes, I agree, we need multiple things: accurate locations from the
> front ends (ideally macro-expansion-aware), start- and end-locations,
> better diagnostic output of various kinds, perhaps smarter location
> handling in the optimizations, and of course finally column output in
> dwarf.  I hope that covers the 80% of stuff we all seem to agree on.

Regarding the list of improvements, I think most of the benefits from 
caret diagnostics can be gained simply by printing accurate texts of 
expressions (or declarations etc.) within the existing one-line diagnostic 
messages, without needing major and incompatible stylistic changes.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics

[Joseph S. Myers]

On Thu, 14 Aug 2008, Ralf Wildenhues wrote:

> * Tom Tromey wrote on Thu, Aug 14, 2008 at 06:52:24PM CEST:
> > I'm sympathetic to the idea that switching to caret output by default
> > will break things.  However, I don't think that GCS-style ranges are
> > necessarily any more reality-proof, because I am skeptical that most
> > tool developers read this document when deciding how to parse GCC's
> > output.
> 
> The GCS document helps not only to let tool developers know how output
> of programs they parse looks like; more importantly, it helps ensure
> that output from different programs is consistent.

And thereby allows a post-processor to add caret diagnostics to the output 
of any GCS-conforming program, or an IDE or editor to show the locations 
of errors from such a program, not just GCC, for example.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics

[Joseph S. Myers]

On Thu, 14 Aug 2008, Tom Tromey wrote:

> Manuel's idea that we should enable column- or caret-output in the
> development (but not release) GCC is worthy of consideration.  We
> certainly aren't seeing much progress on this front as-is, maybe this
> change would inspire GCC developers a bit.  It will also help root out
> the non-GCS-complaint tools ;)

I think making this different in development would be a mistake.  It's not 
like --enable-checking; it's something user-visible.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics

[Chris Lattner]

On Aug 14, 2008, at 8:47 AM, Joseph S. Myers wrote:

> On Thu, 14 Aug 2008, Robert Dewar wrote:
>
>> BTW, I am all in favor of caret output, it's not the default in
>> gnat, the default is more like the C default, but -gnatv gives
>> output like:
>
> And I'd hope we could keep things that way for both C and Ada, but  
> make

Please don't forget C++.

-Chris

Re: [PATCH] caret diagnostics

[Ralf Wildenhues]

* Tom Tromey wrote on Thu, Aug 14, 2008 at 06:52:24PM CEST:
> I'm sympathetic to the idea that switching to caret output by default
> will break things.  However, I don't think that GCS-style ranges are
> necessarily any more reality-proof, because I am skeptical that most
> tool developers read this document when deciding how to parse GCC's
> output.

The GCS document helps not only to let tool developers know how output
of programs they parse looks like; more importantly, it helps ensure
that output from different programs is consistent.

If the GCS-style is not powerful enough to meet GCC's needs, then please
let's not only improve GCC, but the GCS document also, so that other
programs improve compatibly.

Cheers,
Ralf

Re: [PATCH] caret diagnostics

[Tom Tromey]

>>>>> "Joseph" == Joseph S Myers <joseph@codesourcery.com> writes:

Joseph> (b) to print GCS-compliant ranges in text that IDEs can parse
Joseph> to highlight the relevant text in their editors

Joseph> Caret diagnostics are only one of the styles in which the
Joseph> accurate location information can be used, and implementing an
Joseph> individial style is only a small part of the solution.

Yes, I agree, we need multiple things: accurate locations from the
front ends (ideally macro-expansion-aware), start- and end-locations,
better diagnostic output of various kinds, perhaps smarter location
handling in the optimizations, and of course finally column output in
dwarf.  I hope that covers the 80% of stuff we all seem to agree on.

I'm sympathetic to the idea that switching to caret output by default
will break things.  However, I don't think that GCS-style ranges are
necessarily any more reality-proof, because I am skeptical that most
tool developers read this document when deciding how to parse GCC's
output.  (I'm guessing that plain column output is ok, since libcpp
already does that.)

I'd like to see carets on by default as part of a major release -- say
GCC 5.0.  (First mention!!)

Manuel's idea that we should enable column- or caret-output in the
development (but not release) GCC is worthy of consideration.  We
certainly aren't seeing much progress on this front as-is, maybe this
change would inspire GCC developers a bit.  It will also help root out
the non-GCS-complaint tools ;)

Tom

Re: [PATCH] caret diagnostics

[Joseph S. Myers]

On Thu, 14 Aug 2008, Robert Dewar wrote:

> BTW, I am all in favor of caret output, it's not the default in
> gnat, the default is more like the C default, but -gnatv gives
> output like:

And I'd hope we could keep things that way for both C and Ada, but make 
similar modes to the Ada ones available for C (with Ada treating the 
common compiler options as synonyms for the the existing Ada options).  
In fact I'd hope we could get Ada using the shared diagnostic 
infrastructure, i18n support etc., but realise that's a lot of work (a lot 
needed doing just to get *almost all* C diagnostics in suitable shape for 
i18n) and might have issues with using the same Ada sources with different 
compiler back end versions.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics

[Manuel López-Ibáñez]

2008/8/14 Robert Dewar <dewar@adacore.com>:
> Manuel López-Ibáñez wrote:
>
>> My proposal is to enable this while not in release mode (like we
>> enable checks and dump statistics and output from the optimizers), so
>> developers notice wrong locations and open a PR and maybe even fix
>> them. Then maybe let users or distributions configure the default as
>> they wish, with our releases defaulting to off.
>
> Won't this have a negative effect on the test infrastructure? It
> sure would for our Ada testing, where we have hundreds of thousands
> of diagnostic messages in our test suites.

Even when enabled  with -fdiagnostics-show-caret  and configured with
--enable-languages=all,ada,obj-c++, all acats and gnat tests pass. Of
course, we should disable it with -fno-diagnostics-show-caret as we
currently do for -fshow-colum in lib/gcc.exp. You'll notice that is
what my patch does if you read it.

Cheers,

Manuel.

Re: [PATCH] caret diagnostics

[Robert Dewar]

Manuel López-Ibáñez wrote:

> My proposal is to enable this while not in release mode (like we
> enable checks and dump statistics and output from the optimizers), so
> developers notice wrong locations and open a PR and maybe even fix
> them. Then maybe let users or distributions configure the default as
> they wish, with our releases defaulting to off.

Won't this have a negative effect on the test infrastructure? It
sure would for our Ada testing, where we have hundreds of thousands
of diagnostic messages in our test suites.

BTW, I am all in favor of caret output, it's not the default in
gnat, the default is more like the C default, but -gnatv gives
output like:

>     2.    a : integer
>                      |
>        >>> missing ";"
> 
>     4.    a := b c;
>                 |
>        >>> binary operator expected
> 
>     5.    a := b
>                 |
>        >>> missing ";"

and -gnatl gives a complete listing

> Compiling: k.adb
> 
>      1. procedure k is
>      2.    a : integer
>                       |
>         >>> missing ";"
> 
>      3. begin
>      4.    a := b c;
>                  |
>         >>> binary operator expected
> 
>      5.    a := b
>                  |
>         >>> missing ";"
> 
>      6.    c;
>      7. end;

This last listing is interesting, it shows how GNAT pays attention
to code layout in diagnostics, giving quite different messages for
two cases of the same token sequence.

FWIW we find most users are aware of options like this. We make
a special attempt to encourage people to at least read through
the list of switches, even if they read no other documentation.

Re: [PATCH] caret diagnostics

[Manuel López-Ibáñez]

2008/8/14 Robert Dewar <dewar@adacore.com>:
> Manuel López-Ibáñez wrote:
>>
>> 2008/8/14 Joseph S. Myers <joseph@codesourcery.com>:
>>>
>>> But in any case the default should be the default with no configure
>>> option, users liking it should find their makefiles work the same
>>> everywhere and users not liking it can add the opposite option.
>>
>> Then we are not going to get correct locations ever. New users do not
>> read the manual. Neither old users do. New functionality disabled by
>> default will be lost for both. I am fairly sure that a significant
>> percentage of GCC developers (not just users) do not know about
>> -fdiagnostics-show-option.
>
> Users are not beta testers. Forcing inconvenience on users to benefit
> developers is not justified for a relatively minor issue like this.
> Changing the default here would have a huge impact, I think probably
> some developers do not realize the impact that changing messages or
> output has on people developing large systems for which they expect
> stable compiler output, e.g. for test cases.

My proposal is to enable this while not in release mode (like we
enable checks and dump statistics and output from the optimizers), so
developers notice wrong locations and open a PR and maybe even fix
them. Then maybe let users or distributions configure the default as
they wish, with our releases defaulting to off.

Cheers,

Manuel.

Re: [PATCH] caret diagnostics

[Robert Dewar]

Joseph S. Myers wrote:

> We certainly do change the *text* of messages to improve them (this 
> includes putting in more information that can fit within the existing 
> single-line format), and add new messages following the standard formats, 
> but I believe we should leave consumers able to rely on certain aspects of 
> the output that follow the GNU Coding Standards and not start inserting 
> new, non-standard lines in the output that would dominate the standard 
> ones.

Right of course improving messages is always reasonable (though in 
practice at least in the case of Ada, we find that often the major
work in updating text of a message is updating all the test base
lines :-))

I just think that adding carets everywhere by default is too big
a  change to be justified.
> 

Re: [PATCH] caret diagnostics

[Joseph S. Myers]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1587 bytes --]

On Thu, 14 Aug 2008, Robert Dewar wrote:

> Manuel López-Ibáñez wrote:
> > 2008/8/14 Joseph S. Myers <joseph@codesourcery.com>:
> > > But in any case the default should be the default with no configure
> > > option, users liking it should find their makefiles work the same
> > > everywhere and users not liking it can add the opposite option.
> > 
> > Then we are not going to get correct locations ever. New users do not
> > read the manual. Neither old users do. New functionality disabled by
> > default will be lost for both. I am fairly sure that a significant
> > percentage of GCC developers (not just users) do not know about
> > -fdiagnostics-show-option.
> 
> Users are not beta testers. Forcing inconvenience on users to benefit
> developers is not justified for a relatively minor issue like this.
> Changing the default here would have a huge impact, I think probably
> some developers do not realize the impact that changing messages or
> output has on people developing large systems for which they expect
> stable compiler output, e.g. for test cases.

We certainly do change the *text* of messages to improve them (this 
includes putting in more information that can fit within the existing 
single-line format), and add new messages following the standard formats, 
but I believe we should leave consumers able to rely on certain aspects of 
the output that follow the GNU Coding Standards and not start inserting 
new, non-standard lines in the output that would dominate the standard 
ones.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics

[Robert Dewar]

Manuel López-Ibáñez wrote:
> 2008/8/14 Joseph S. Myers <joseph@codesourcery.com>:
>> But in any case the default should be the default with no configure
>> option, users liking it should find their makefiles work the same
>> everywhere and users not liking it can add the opposite option.
> 
> Then we are not going to get correct locations ever. New users do not
> read the manual. Neither old users do. New functionality disabled by
> default will be lost for both. I am fairly sure that a significant
> percentage of GCC developers (not just users) do not know about
> -fdiagnostics-show-option.

Users are not beta testers. Forcing inconvenience on users to benefit
developers is not justified for a relatively minor issue like this.
Changing the default here would have a huge impact, I think probably
some developers do not realize the impact that changing messages or
output has on people developing large systems for which they expect
stable compiler output, e.g. for test cases.