[PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Manuel López-Ibáñez]

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

2008/8/14 Tom Tromey <tromey@redhat.com>:
>
> ISTR Manuel having a patch for caret diagnostic output... ?
>

I was planning to submit it this week to consider it for GCC 4.4
(disabled by default). I am still testing it. Bootstrap and regression
testing with --enable-languages=all,ada,obj-c++ is very slow even on a
x86_64-unknown-gnu-linux-pc.  But here it is for your reviewing and
testing pleasure. ;-)

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

Alternative approaches are:

b) Re-open the file and fseek but that is not trivial since we need to
do it fast but still do all character conversions that we did when
libcpp opened it the first time. This is approximately what clang does
as far as I know except that they keep a cache of buffers ever
reopened. I think that thanks to our line-maps implementation, we can
do the seeking quite more efficiently in terms of computation time.
Still I do not know how to *properly* and *efficiently* re-open a
file.

c) Memcpy the relevant lines to a buffer for each line_map. This does
not seem a good approach under any circumstance. It means more memory
needed while the input buffers are opened (because we will end up with
two copies of the buffer) and we cannot free up that memory later
because we do not track how many references there are to each
line_maps, so we never free them. Perhaps the GC can help here? This
is the approach followed by GFortran.

Comments are welcome. However, I don't have time to do
memory/computation time tests (setting up the environment itself seems
very time consuming), so those would be *greatly* appreciated!

You can see many examples of the caret output by configuring with
--enable-caret-diagnostics, then reverting the changes to
gcc/testsuite/lib/gcc.exp and running the testsuite. Check the output
in the gcc.log and g++.log files.

Cheers,

Manuel.

[-- Attachment #2: caret-diagnostics-20080814.diff --]
[-- Type: text/plain, Size: 38186 bytes --]

Index: gcc/java/jcf-parse.c
===================================================================
--- gcc/java/jcf-parse.c	(revision 138935)
+++ gcc/java/jcf-parse.c	(working copy)
@@ -1193,12 +1193,14 @@ give_name_to_class (JCF *jcf, int i)
 					    JPOOL_UTF_LENGTH (jcf, j));
       this_class = lookup_class (class_name);
       {
       tree source_name = identifier_subst (class_name, "", '.', '/', ".java");
       const char *sfname = find_sourcefile (IDENTIFIER_POINTER (source_name));
-      linemap_add (line_table, LC_ENTER, false, sfname, 0);
-      input_location = linemap_line_start (line_table, 0, 1);
+      /* FIXME CARET: We should add a pointer to the input line
+	 instead of NULL.  */
+      linemap_add (line_table, LC_ENTER, false, sfname, 0, NULL);
+      input_location = linemap_line_start (line_table, 0, 1, NULL);
       file_start_location = input_location;
       DECL_SOURCE_LOCATION (TYPE_NAME (this_class)) = input_location;
       if (main_input_filename == NULL && jcf == main_jcf)
 	main_input_filename = sfname;
       }
@@ -1472,11 +1474,11 @@ jcf_parse (JCF* jcf)
     fatal_error ("error while parsing final attributes");
 
   if (TYPE_REFLECTION_DATA (current_class))
     annotation_write_byte (JV_DONE_ATTR);
 
-  linemap_add (line_table, LC_LEAVE, false, NULL, 0);
+  linemap_add (line_table, LC_LEAVE, false, NULL, 0, NULL);
 
   /* The fields of class_type_node are already in correct order. */
   if (current_class != class_type_node && current_class != object_type_node)
     TYPE_FIELDS (current_class) = nreverse (TYPE_FIELDS (current_class));
 
@@ -1505,12 +1507,12 @@ load_inner_classes (tree cur_class)
 
 static void
 duplicate_class_warning (const char *filename)
 {
   location_t warn_loc;
-  linemap_add (line_table, LC_RENAME, 0, filename, 0);
-  warn_loc = linemap_line_start (line_table, 0, 1);
+  linemap_add (line_table, LC_RENAME, 0, filename, 0, NULL);
+  warn_loc = linemap_line_start (line_table, 0, 1, NULL);
   warning (0, "%Hduplicate class will only be compiled once", &warn_loc);
 }
 
 static void
 java_layout_seen_class_methods (void)
@@ -1558,11 +1560,11 @@ parse_class_file (void)
 
   input_location = DECL_SOURCE_LOCATION (TYPE_NAME (current_class));
   {
     /* Re-enter the current file.  */
     expanded_location loc = expand_location (input_location);
-    linemap_add (line_table, LC_ENTER, 0, loc.file, loc.line);
+    linemap_add (line_table, LC_ENTER, 0, loc.file, loc.line, NULL);
   }
   file_start_location = input_location;
   (*debug_hooks->start_source_file) (input_line, input_filename);
 
   java_mark_class_local (current_class);
@@ -1625,11 +1627,11 @@ parse_class_file (void)
 	       * Needs to be set before init_function_start. */
 	      if (min_line == 0 || line < min_line)
 		min_line = line;
 	    }
 	  if (min_line != 0)
-	    input_location = linemap_line_start (line_table, min_line, 1);
+	    input_location = linemap_line_start (line_table, min_line, 1, NULL);
 	}
       else
 	{
 	  linenumber_table = NULL;
 	  linenumber_count = 0;
@@ -1897,18 +1899,18 @@ java_parse_file (int set_yydebug ATTRIBU
 	{
 	  main_jcf = GGC_NEW (JCF);
 	  JCF_ZERO (main_jcf);
 	  main_jcf->read_state = finput;
 	  main_jcf->filbuf = jcf_filbuf_from_stdio;
-	  linemap_add (line_table, LC_ENTER, false, filename, 0);
-	  input_location = linemap_line_start (line_table, 0, 1);
+	  linemap_add (line_table, LC_ENTER, false, filename, 0, NULL);
+	  input_location = linemap_line_start (line_table, 0, 1, NULL);
 	  if (open_in_zip (main_jcf, filename, NULL, 0) <  0)
 	    fatal_error ("bad zip/jar file %s", filename);
 	  localToFile = SeenZipFiles;
 	  /* Register all the classes defined there.  */
 	  process_zip_dir ((FILE *) main_jcf->read_state);
-	  linemap_add (line_table, LC_LEAVE, false, NULL, 0);
+	  linemap_add (line_table, LC_LEAVE, false, NULL, 0, NULL);
 	  parse_zip_file_entries ();
 	}
       else if (magic == (JCF_u4) ZIPEMPTYMAGIC)
 	{
 	  /* Ignore an empty input jar.  */
@@ -1921,11 +1923,11 @@ java_parse_file (int set_yydebug ATTRIBU
 	  java_parser_context_save_global ();
 
 	  parse_source_file_1 (real_file, filename, finput);
 	  java_parser_context_restore_global ();
 	  java_pop_parser_context (1);
-	  linemap_add (line_table, LC_LEAVE, false, NULL, 0);
+	  linemap_add (line_table, LC_LEAVE, false, NULL, 0, NULL);
 #endif
 	}
     }
 
   for (node = current_file_list; node; node = TREE_CHAIN (node))
Index: gcc/java/expr.c
===================================================================
--- gcc/java/expr.c	(revision 138935)
+++ gcc/java/expr.c	(working copy)
@@ -3229,11 +3229,11 @@ expand_byte_code (JCF *jcf, tree method)
 	      int pc = GET_u2 (linenumber_pointer);
 	      linenumber_pointer += 4;
 	      if (pc == PC)
 		{
 		  int line = GET_u2 (linenumber_pointer - 2);
-		  input_location = linemap_line_start (line_table, line, 1);
+		  input_location = linemap_line_start (line_table, line, 1, NULL);
 		  if (input_location > max_location)
 		    max_location = input_location;
 		  if (!(instruction_bits[PC] & BCODE_HAS_MULTI_LINENUMBERS))
 		    break;
 		}
Index: gcc/java/lang.c
===================================================================
--- gcc/java/lang.c	(revision 138935)
+++ gcc/java/lang.c	(working copy)
@@ -602,12 +602,12 @@ java_post_options (const char **pfilenam
 		  free (buf);
 		}
 	    }
 	}
     }
-  linemap_add (line_table, LC_ENTER, false, filename, 0);
-  linemap_add (line_table, LC_RENAME, false, "<built-in>", 0);
+  linemap_add (line_table, LC_ENTER, false, filename, 0, NULL);
+  linemap_add (line_table, LC_RENAME, false, "<built-in>", 0, NULL);
 
   /* Initialize the compiler back end.  */
   return false;
 }
 
Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 138935)
+++ gcc/tree.c	(working copy)
@@ -3516,18 +3516,24 @@ expand_location (source_location loc)
   if (loc == 0)
     {
       xloc.file = NULL;
       xloc.line = 0;
       xloc.column = 0;
+#ifdef USE_CARET_DIAGNOSTICS
+      xloc.linebuf = NULL;
+#endif
       xloc.sysp = 0;
     }
   else
     {
       const struct line_map *map = linemap_lookup (line_table, loc);
       xloc.file = map->to_file;
       xloc.line = SOURCE_LINE (map, loc);
       xloc.column = SOURCE_COLUMN (map, loc);
+#ifdef USE_CARET_DIAGNOSTICS
+      xloc.linebuf = SOURCE_LINEBUFFER (map, loc);
+#endif
       xloc.sysp = map->sysp != 0;
     };
   return xloc;
 }
 
Index: gcc/configure
===================================================================
--- gcc/configure	(revision 138935)
+++ gcc/configure	(working copy)
@@ -1011,10 +1011,13 @@ Optional Features:
   --enable-generated-files-in-srcdir
                           put copies of generated files in source dir
                           intended for creating source tarballs for users
                           without texinfo bison or flex.
   --enable-werror-always  enable -Werror despite compiler version
+  --enable-caret-diagnostics
+                          enable printing the source code causing a diagnostic
+			  with a caret symbol indicating the exact location.
   --enable-checking=LIST
 			  enable expensive run-time checks.  With LIST,
 			  enable only specific categories of checks.
 			  Categories are: yes,no,all,none,release.
 			  Flags are: assert,df,fold,gc,gcac,gimple,misc,
@@ -7232,10 +7235,26 @@ warn_cflags=
 if test "x$GCC" = "xyes"; then
   warn_cflags='$(GCC_WARN_CFLAGS)'
 fi
 
 
+# Check whether --enable-caret-diagnostics or --disable-caret-diagnostics was given.
+if test "${enable_caret_diagnostics+set}" = set; then
+  enableval="$enable_caret_diagnostics"
+
+else
+  enable_caret_diagnostics=no
+fi;
+
+if test x$enable_caret_diagnostics = xyes ; then
+
+cat >>confdefs.h <<\_ACEOF
+#define USE_CARET_DIAGNOSTICS 1
+_ACEOF
+
+fi
+
 # Enable expensive internal checks
 is_release=
 if test x"`cat $srcdir/DEV-PHASE`" != xexperimental; then
   is_release=yes
 fi
@@ -14719,17 +14738,17 @@ echo $ECHO_N "checking the name lister (
 if test "${lt_cv_nm_interface+set}" = set; then
   echo $ECHO_N "(cached) $ECHO_C" >&6
 else
   lt_cv_nm_interface="BSD nm"
   echo "int some_variable = 0;" > conftest.$ac_ext
-  (eval echo "\"\$as_me:14724: $ac_compile\"" >&5)
+  (eval echo "\"\$as_me:14743: $ac_compile\"" >&5)
   (eval "$ac_compile" 2>conftest.err)
   cat conftest.err >&5
-  (eval echo "\"\$as_me:14727: $NM \\\"conftest.$ac_objext\\\"\"" >&5)
+  (eval echo "\"\$as_me:14746: $NM \\\"conftest.$ac_objext\\\"\"" >&5)
   (eval "$NM \"conftest.$ac_objext\"" 2>conftest.err > conftest.out)
   cat conftest.err >&5
-  (eval echo "\"\$as_me:14730: output\"" >&5)
+  (eval echo "\"\$as_me:14749: output\"" >&5)
   cat conftest.out >&5
   if $GREP 'External.*some_variable' conftest.out > /dev/null; then
     lt_cv_nm_interface="MS dumpbin"
   fi
   rm -f conftest*
@@ -15780,11 +15799,11 @@ ia64-*-hpux*)
   fi
   rm -rf conftest*
   ;;
 *-*-irix6*)
   # Find out which ABI we are using.
-  echo '#line 15785 "configure"' > conftest.$ac_ext
+  echo '#line 15804 "configure"' > conftest.$ac_ext
   if { (eval echo "$as_me:$LINENO: \"$ac_compile\"") >&5
   (eval $ac_compile) 2>&5
   ac_status=$?
   echo "$as_me:$LINENO: \$? = $ac_status" >&5
   (exit $ac_status); }; then
@@ -16400,15 +16419,15 @@ else
    # The option is referenced via a variable to avoid confusing sed.
    lt_compile=`echo "$ac_compile" | $SED \
    -e 's:.*FLAGS}\{0,1\} :&$lt_compiler_flag :; t' \
    -e 's: [^ ]*conftest\.: $lt_compiler_flag&:; t' \
    -e 's:$: $lt_compiler_flag:'`
-   (eval echo "\"\$as_me:16405: $lt_compile\"" >&5)
+   (eval echo "\"\$as_me:16424: $lt_compile\"" >&5)
    (eval "$lt_compile" 2>conftest.err)
    ac_status=$?
    cat conftest.err >&5
-   echo "$as_me:16409: \$? = $ac_status" >&5
+   echo "$as_me:16428: \$? = $ac_status" >&5
    if (exit $ac_status) && test -s "$ac_outfile"; then
      # The compiler can only warn and ignore the option if not recognized
      # So say no if there are warnings other than the usual output.
      $ECHO "X$_lt_compiler_boilerplate" | $Xsed -e '/^$/d' >conftest.exp
      $SED '/^$/d; /^ *+/d' conftest.err >conftest.er2
@@ -16722,15 +16741,15 @@ else
    # The option is referenced via a variable to avoid confusing sed.
    lt_compile=`echo "$ac_compile" | $SED \
    -e 's:.*FLAGS}\{0,1\} :&$lt_compiler_flag :; t' \
    -e 's: [^ ]*conftest\.: $lt_compiler_flag&:; t' \
    -e 's:$: $lt_compiler_flag:'`
-   (eval echo "\"\$as_me:16727: $lt_compile\"" >&5)
+   (eval echo "\"\$as_me:16746: $lt_compile\"" >&5)
    (eval "$lt_compile" 2>conftest.err)
    ac_status=$?
    cat conftest.err >&5
-   echo "$as_me:16731: \$? = $ac_status" >&5
+   echo "$as_me:16750: \$? = $ac_status" >&5
    if (exit $ac_status) && test -s "$ac_outfile"; then
      # The compiler can only warn and ignore the option if not recognized
      # So say no if there are warnings other than the usual output.
      $ECHO "X$_lt_compiler_boilerplate" | $Xsed -e '/^$/d' >conftest.exp
      $SED '/^$/d; /^ *+/d' conftest.err >conftest.er2
@@ -16827,15 +16846,15 @@ else
    # with a dollar sign (not a hyphen), so the echo should work correctly.
    lt_compile=`echo "$ac_compile" | $SED \
    -e 's:.*FLAGS}\{0,1\} :&$lt_compiler_flag :; t' \
    -e 's: [^ ]*conftest\.: $lt_compiler_flag&:; t' \
    -e 's:$: $lt_compiler_flag:'`
-   (eval echo "\"\$as_me:16832: $lt_compile\"" >&5)
+   (eval echo "\"\$as_me:16851: $lt_compile\"" >&5)
    (eval "$lt_compile" 2>out/conftest.err)
    ac_status=$?
    cat out/conftest.err >&5
-   echo "$as_me:16836: \$? = $ac_status" >&5
+   echo "$as_me:16855: \$? = $ac_status" >&5
    if (exit $ac_status) && test -s out/conftest2.$ac_objext
    then
      # The compiler can only warn and ignore the option if not recognized
      # So say no if there are warnings
      $ECHO "X$_lt_compiler_boilerplate" | $Xsed -e '/^$/d' > out/conftest.exp
@@ -16882,15 +16901,15 @@ else
    # with a dollar sign (not a hyphen), so the echo should work correctly.
    lt_compile=`echo "$ac_compile" | $SED \
    -e 's:.*FLAGS}\{0,1\} :&$lt_compiler_flag :; t' \
    -e 's: [^ ]*conftest\.: $lt_compiler_flag&:; t' \
    -e 's:$: $lt_compiler_flag:'`
-   (eval echo "\"\$as_me:16887: $lt_compile\"" >&5)
+   (eval echo "\"\$as_me:16906: $lt_compile\"" >&5)
    (eval "$lt_compile" 2>out/conftest.err)
    ac_status=$?
    cat out/conftest.err >&5
-   echo "$as_me:16891: \$? = $ac_status" >&5
+   echo "$as_me:16910: \$? = $ac_status" >&5
    if (exit $ac_status) && test -s out/conftest2.$ac_objext
    then
      # The compiler can only warn and ignore the option if not recognized
      # So say no if there are warnings
      $ECHO "X$_lt_compiler_boilerplate" | $Xsed -e '/^$/d' > out/conftest.exp
@@ -19679,11 +19698,11 @@ else
   lt_cv_dlopen_self=cross
 else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 19684 "configure"
+#line 19703 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
 #include <dlfcn.h>
 #endif
@@ -19779,11 +19798,11 @@ else
   lt_cv_dlopen_self_static=cross
 else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 19784 "configure"
+#line 19803 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
 #include <dlfcn.h>
 #endif
Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 138935)
+++ gcc/diagnostic.c	(working copy)
@@ -103,10 +103,13 @@ diagnostic_initialize (diagnostic_contex
   memset (context->diagnostic_count, 0, sizeof context->diagnostic_count);
   context->issue_warnings_are_errors_message = true;
   context->warning_as_error_requested = false;
   memset (context->classify_diagnostic, DK_UNSPECIFIED,
 	  sizeof context->classify_diagnostic);
+#ifdef USE_CARET_DIAGNOSTICS
+  context->show_caret = true;
+#endif
   context->show_option_requested = false;
   context->abort_on_error = false;
   context->internal_error = NULL;
   diagnostic_starter (context) = default_diagnostic_starter;
   diagnostic_finalizer (context) = default_diagnostic_finalizer;
@@ -161,10 +164,47 @@ diagnostic_build_prefix (diagnostic_info
      : flag_show_column && s.column != 0
      ? 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));
 }
 
+#ifdef USE_CARET_DIAGNOSTICS
+static const unsigned char *
+get_source_line (expanded_location s)
+{
+  return s.linebuf;
+}
+
+static void
+diagnostic_show_locus (diagnostic_context * context ATTRIBUTE_UNUSED,
+		       diagnostic_info *diagnostic)
+{
+  const unsigned char *line;
+  int max_width = 80;
+  expanded_location s;
+
+  if (!context->show_caret)
+    return;
+
+  s = expand_location (diagnostic->location);
+  line = get_source_line (s);
+  if (line == NULL)
+    return;
+
+  putchar (' ');
+
+  while (max_width > 0 && *line != '\0' && *line != '\n')
+    {
+      max_width--;
+      putchar (*line);
+      line++;
+    }
+  putchar('\n');
+  gcc_assert (s.column > 0);
+  printf (" %*c\n", s.column, '^');
+}
+#endif
+
 /* 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)
@@ -402,10 +442,13 @@ 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);
+#ifdef USE_CARET_DIAGNOSTICS
+  diagnostic_show_locus (context, diagnostic);
+#endif
   diagnostic_action_after_output (context, diagnostic);
   diagnostic->message.format_spec = saved_format_spec;
   diagnostic->abstract_origin = NULL;
 
   context->lock--;
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 138935)
+++ gcc/diagnostic.h	(working copy)
@@ -82,10 +82,16 @@ struct diagnostic_context
 
   /* True if we should print the command line option which controls
      each diagnostic, if known.  */
   bool show_option_requested;
 
+#ifdef USE_CARET_DIAGNOSTICS
+  /* True if we should print the source line with a caret indicating
+     the location.  */
+  bool show_caret;
+#endif
+
   /* True if we should raise a SIGABRT on errors.  */
   bool abort_on_error;
 
   /* This function is called before any message is printed out.  It is
      responsible for preparing message prefix and such.  For example, it
Index: gcc/input.h
===================================================================
--- gcc/input.h	(revision 138935)
+++ gcc/input.h	(working copy)
@@ -40,10 +40,15 @@ typedef struct GTY (())
   /* The line-location in the source file.  */
   int line;
 
   int column;
 
+#ifdef USE_CARET_DIAGNOSTICS
+  /* The offset location in the source file.  */
+  const unsigned char *linebuf;
+#endif
+
   /* In a system header?. */
   bool sysp;
 } expanded_location;
 
 extern expanded_location expand_location (source_location);
Index: gcc/testsuite/lib/gcc.exp
===================================================================
--- gcc/testsuite/lib/gcc.exp	(revision 138935)
+++ gcc/testsuite/lib/gcc.exp	(working copy)
@@ -148,10 +148,10 @@ proc gcc_target_compile { source dest ty
 	set options [concat "{additional_flags=$TOOL_OPTIONS}" $options]
     }
     if [target_info exists gcc,timeout] {
 	lappend options "timeout=[target_info gcc,timeout]"
     }
-    lappend options "additional_flags=-fno-show-column"
+    lappend options "additional_flags=-fno-show-column -fno-diagnostics-show-caret"
     lappend options "compiler=$GCC_UNDER_TEST"
     set options [dg-additional-files-options $options $source]
     return [target_compile $source $dest $type $options]
 }
Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(revision 138935)
+++ gcc/cp/decl.c	(working copy)
@@ -11898,11 +11898,11 @@ finish_function (int flags)
 #endif
 	  /* Hack.  We don't want the middle-end to warn that this
 	     return is unreachable, so put the statement on the
 	     special line 0.  */
 	  {
-	    location_t linezero = linemap_line_start (line_table, 0, 1);
+	    location_t linezero = linemap_line_start (line_table, 0, 1, NULL);
 	    SET_EXPR_LOCATION (stmt, linezero);
 	  }
 	}
 
       if (use_eh_spec_block (current_function_decl))
Index: gcc/c-tree.h
===================================================================
--- gcc/c-tree.h	(revision 138935)
+++ gcc/c-tree.h	(working copy)
@@ -460,11 +460,11 @@ extern void c_init_decl_processing (void
 extern void c_dup_lang_specific_decl (tree);
 extern void c_print_identifier (FILE *, tree, int);
 extern int quals_from_declspecs (const struct c_declspecs *);
 extern struct c_declarator *build_array_declarator (tree, struct c_declspecs *,
 						    bool, bool);
-extern tree build_enumerator (struct c_enum_contents *, tree, tree);
+extern tree build_enumerator (struct c_enum_contents *, tree, tree, location_t);
 extern tree check_for_loop_decls (void);
 extern void mark_forward_parm_decls (void);
 extern void declare_parm_level (void);
 extern void undeclared_variable (tree, location_t);
 extern tree declare_label (tree);
Index: gcc/config.in
===================================================================
--- gcc/config.in	(revision 138935)
+++ gcc/config.in	(working copy)
@@ -375,15 +375,17 @@
 /* Define if your assembler supports thread-local storage. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_AS_TLS
 #endif
 
+
 /* Define if your assembler supports VSX instructions. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_AS_VSX
 #endif
 
+
 /* Define to 1 if you have the `atoll' function. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_ATOLL
 #endif
 
@@ -1464,10 +1466,17 @@
 #ifndef USED_FOR_TARGET
 #undef USE_AS_TRADITIONAL_FORMAT
 #endif
 
 
+/* Define if diagnostics should print source line with caret symbol indicating
+   exact location. */
+#ifndef USED_FOR_TARGET
+#undef USE_CARET_DIAGNOSTICS
+#endif
+
+
 /* Define to 1 if the 'long long' (or '__int64') is wider than 'long' but
    still efficiently supported by the host hardware. */
 #ifndef USED_FOR_TARGET
 #undef USE_LONG_LONG_FOR_WIDEST_FAST_INT
 #endif
Index: gcc/c-pch.c
===================================================================
--- gcc/c-pch.c	(revision 138935)
+++ gcc/c-pch.c	(working copy)
@@ -427,11 +427,11 @@ c_common_read_pch (cpp_reader *pfile, co
 
   fclose (f);
 
   line_table->trace_includes = saved_trace_includes;
   cpp_set_line_map (pfile, line_table);
-  linemap_add (line_table, LC_RENAME, 0, saved_loc.file, saved_loc.line);
+  linemap_add (line_table, LC_RENAME, 0, saved_loc.file, saved_loc.line, NULL);
 
   /* Give the front end a chance to take action after a PCH file has
      been loaded.  */
   if (lang_post_pch_load)
     (*lang_post_pch_load) ();
Index: gcc/opts.c
===================================================================
--- gcc/opts.c	(revision 138935)
+++ gcc/opts.c	(working copy)
@@ -1743,13 +1743,24 @@ common_handle_option (size_t scode, cons
       else
 	return 0;
       break;
 
     case OPT_fdiagnostics_show_option:
-      global_dc->show_option_requested = true;
+      global_dc->show_option_requested = value;
       break;
 
+    case OPT_fdiagnostics_show_caret:
+#ifdef USE_CARET_DIAGNOSTICS
+      global_dc->show_caret = value;
+#else
+      if (value)
+	error ("-fdiagnostics-show-caret is disabled. " 
+	       "Configure GCC with --enable-caret-diagnostics");
+#endif
+      break;
+
+
     case OPT_fdump_:
       if (!dump_switch_p (arg))
 	return 0;
       break;
 
Index: gcc/ada/gcc-interface/trans.c
===================================================================
--- gcc/ada/gcc-interface/trans.c	(revision 138935)
+++ gcc/ada/gcc-interface/trans.c	(working copy)
@@ -272,14 +272,14 @@ gigi (Node_Id gnat_root, int max_gnat_no
       /* We rely on the order isomorphism between files and line maps.  */
       gcc_assert ((int) line_table->used == i);
 
       /* We create the line map for a source file at once, with a fixed number
 	 of columns chosen to avoid jumping over the next power of 2.  */
-      linemap_add (line_table, LC_ENTER, 0, filename, 1);
-      linemap_line_start (line_table, file_info_ptr[i].Num_Source_Lines, 252);
+      linemap_add (line_table, LC_ENTER, 0, filename, 1, NULL);
+      linemap_line_start (line_table, file_info_ptr[i].Num_Source_Lines, 252, NULL);
       linemap_position_for_column (line_table, 252 - 1);
-      linemap_add (line_table, LC_LEAVE, 0, NULL, 0);
+      linemap_add (line_table, LC_LEAVE, 0, NULL, 0, NULL);
     }
 
   /* Initialize ourselves.  */
   init_code_table ();
   init_gnat_to_gnu ();
Index: gcc/c-decl.c
===================================================================
--- gcc/c-decl.c	(revision 138935)
+++ gcc/c-decl.c	(working copy)
@@ -5931,11 +5931,11 @@ finish_enum (tree enumtype, tree values,
    current enumeration type (one that was begun with start_enum).
    Return a tree-list containing the CONST_DECL and its value.
    Assignment of sequential values by default is handled here.  */
 
 tree
-build_enumerator (struct c_enum_contents *the_enum, tree name, tree value)
+build_enumerator (struct c_enum_contents *the_enum, tree name, tree value, location_t value_loc)
 {
   tree decl, type;
 
   /* Validate and default VALUE.  */
 
@@ -5968,11 +5968,12 @@ build_enumerator (struct c_enum_contents
 	error ("overflow in enumeration values");
     }
 
   if (pedantic && !int_fits_type_p (value, integer_type_node))
     {
-      pedwarn (OPT_pedantic, "ISO C restricts enumerator values to range of %<int%>");
+      pedwarn (OPT_pedantic, "%HISO C restricts enumerator values to range of %<int%>", 
+	       &value_loc);
       /* XXX This causes -pedantic to change the meaning of the program.
 	 Remove?  -zw 2004-03-15  */
       value = convert (integer_type_node, value);
     }
 
Index: gcc/fortran/scanner.c
===================================================================
--- gcc/fortran/scanner.c	(revision 138935)
+++ gcc/fortran/scanner.c	(working copy)
@@ -1509,11 +1509,11 @@ get_file (const char *name, enum lc_reas
 
   f->up = current_file;
   if (current_file != NULL)
     f->inclusion_line = current_file->line;
 
-  linemap_add (line_table, reason, false, f->filename, 1);
+  linemap_add (line_table, reason, false, f->filename, 1, NULL);
 
   return f;
 }
 
 
@@ -1642,11 +1642,11 @@ preprocessor_line (gfc_char_t *c)
 	}
 
       add_file_change (NULL, line);
       current_file = current_file->up;
       linemap_add (line_table, LC_RENAME, false, current_file->filename,
-		   current_file->line);
+		   current_file->line, NULL);
     }
 
   /* The name of the file can be a temporary file produced by
      cpp. Replace the name if it is different.  */
 
@@ -1885,11 +1885,11 @@ load_file (const char *realfilename, con
 
       b = (gfc_linebuf *) gfc_getmem (gfc_linebuf_header_size
 				      + (len + 1) * sizeof (gfc_char_t));
 
       b->location
-	= linemap_line_start (line_table, current_file->line++, 120);
+	= linemap_line_start (line_table, current_file->line++, 120, NULL);
       b->file = current_file;
       b->truncated = trunc;
       wide_strcpy (b->line, line);
 
       if (line_head == NULL)
@@ -1909,11 +1909,11 @@ load_file (const char *realfilename, con
   fclose (input);
 
   if (!initial)
     add_file_change (NULL, current_file->inclusion_line + 1);
   current_file = current_file->up;
-  linemap_add (line_table, LC_LEAVE, 0, NULL, 0);
+  linemap_add (line_table, LC_LEAVE, 0, NULL, 0, NULL);
   return SUCCESS;
 }
 
 
 /* Open a new file and start scanning from that file. Returns SUCCESS
Index: gcc/fortran/f95-lang.c
===================================================================
--- gcc/fortran/f95-lang.c	(revision 138935)
+++ gcc/fortran/f95-lang.c	(working copy)
@@ -250,12 +250,12 @@ gfc_be_parse_file (int set_yydebug ATTRI
 static bool
 gfc_init (void)
 {
   if (!gfc_cpp_enabled ())
     {
-      linemap_add (line_table, LC_ENTER, false, gfc_source_file, 1);
-      linemap_add (line_table, LC_RENAME, false, "<built-in>", 0);
+      linemap_add (line_table, LC_ENTER, false, gfc_source_file, 1, NULL);
+      linemap_add (line_table, LC_RENAME, false, "<built-in>", 0, NULL);
     }
   else
     gfc_cpp_init_0 ();
 
   gfc_init_decl_processing ();
Index: gcc/configure.ac
===================================================================
--- gcc/configure.ac	(revision 138935)
+++ gcc/configure.ac	(working copy)
@@ -340,10 +340,21 @@ warn_cflags=
 if test "x$GCC" = "xyes"; then
   warn_cflags='$(GCC_WARN_CFLAGS)'
 fi
 AC_SUBST(warn_cflags)
 
+AC_ARG_ENABLE(caret-diagnostics,
+[  --enable-caret-diagnostics
+                          enable printing the source code causing a diagnostic
+			  with a caret symbol indicating the exact location.], [],
+[enable_caret_diagnostics=no])
+
+if test x$enable_caret_diagnostics = xyes ; then
+  AC_DEFINE(USE_CARET_DIAGNOSTICS, 1,
+  [Define if diagnostics should print source line with caret symbol indicating exact location.])
+fi
+
 # Enable expensive internal checks
 is_release=
 if test x"`cat $srcdir/DEV-PHASE`" != xexperimental; then
   is_release=yes
 fi
Index: gcc/c-opts.c
===================================================================
--- gcc/c-opts.c	(revision 138935)
+++ gcc/c-opts.c	(working copy)
@@ -1446,11 +1446,11 @@ finish_options (void)
     {
       size_t i;
 
       cb_file_change (parse_in,
 		      linemap_add (line_table, LC_RENAME, 0,
-				   _("<built-in>"), 0));
+				   _("<built-in>"), 0, NULL));
 
       cpp_init_builtins (parse_in, flag_hosted);
       c_cpp_builtins (parse_in);
 
       /* We're about to send user input to cpplib, so make it warn for
@@ -1464,11 +1464,11 @@ finish_options (void)
 	 their acceptance on the -std= setting.  */
       cpp_opts->warn_dollars = (cpp_opts->pedantic && !cpp_opts->c99);
 
       cb_file_change (parse_in,
 		      linemap_add (line_table, LC_RENAME, 0,
-				   _("<command-line>"), 0));
+				   _("<command-line>"), 0, NULL));
 
       for (i = 0; i < deferred_count; i++)
 	{
 	  struct deferred_opt *opt = &deferred_opts[i];
 
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 138935)
+++ gcc/common.opt	(working copy)
@@ -445,10 +445,14 @@ Common Joined RejectNegative
 
 fdiagnostics-show-option
 Common
 Amend appropriate diagnostic messages with the command line option that controls them
 
+fdiagnostics-show-caret
+Common
+Show the source line with a caret indicating the column
+
 fdump-
 Common Joined RejectNegative
 -fdump-<type>	Dump various compiler internals to a file
 
 fdump-noaddr
Index: gcc/c-parser.c
===================================================================
--- gcc/c-parser.c	(revision 138935)
+++ gcc/c-parser.c	(working copy)
@@ -1639,19 +1639,21 @@ c_parser_enum_specifier (c_parser *parse
 	    }
 	  token = c_parser_peek_token (parser);
 	  enum_id = token->value;
 	  /* Set the location in case we create a decl now.  */
 	  c_parser_set_source_position_from_token (token);
+	  comma_loc = c_parser_peek_token (parser)->location;
 	  c_parser_consume_token (parser);
 	  if (c_parser_next_token_is (parser, CPP_EQ))
 	    {
 	      c_parser_consume_token (parser);
+
 	      enum_value = c_parser_expr_no_commas (parser, NULL).value;
 	    }
 	  else
 	    enum_value = NULL_TREE;
-	  enum_decl = build_enumerator (&the_enum, enum_id, enum_value);
+	  enum_decl = build_enumerator (&the_enum, enum_id, enum_value, comma_loc);
 	  TREE_CHAIN (enum_decl) = values;
 	  values = enum_decl;
 	  seen_comma = false;
 	  if (c_parser_next_token_is (parser, CPP_COMMA))
 	    {
Index: libcpp/directives.c
===================================================================
--- libcpp/directives.c	(revision 138935)
+++ libcpp/directives.c	(working copy)
@@ -1011,14 +1011,17 @@ do_linemarker (cpp_reader *pfile)
 void
 _cpp_do_file_change (cpp_reader *pfile, enum lc_reason reason,
 		     const char *to_file, linenum_type file_line,
 		     unsigned int sysp)
 {
-  const struct line_map *map = linemap_add (pfile->line_table, reason, sysp,
-					    to_file, file_line);
+  const struct line_map *map;
+  const unsigned char *linebuf = (pfile->buffer == NULL) ? NULL : pfile->buffer->next_line;
+
+  map = linemap_add (pfile->line_table, reason, sysp,
+		     to_file, file_line, linebuf);
   if (map != NULL)
-    linemap_line_start (pfile->line_table, map->to_line, 127);
+    linemap_line_start (pfile->line_table, map->to_line, 127, linebuf);
 
   if (pfile->cb.file_change)
     pfile->cb.file_change (pfile, map);
 }
 
Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c	(revision 138935)
+++ libcpp/line-map.c	(working copy)
@@ -83,11 +83,12 @@ linemap_free (struct line_maps *set)
    function.  A call to this function can relocate the previous set of
    maps, so any stored line_map pointers should not be used.  */
 
 const struct line_map *
 linemap_add (struct line_maps *set, enum lc_reason reason,
-	     unsigned int sysp, const char *to_file, linenum_type to_line)
+	     unsigned int sysp, const char *to_file, linenum_type to_line, 
+	     const unsigned char ARG_UNUSED (*linebuf))
 {
   struct line_map *map;
   source_location start_location = set->highest_location + 1;
 
   if (set->used && start_location < set->maps[set->used - 1].start_location)
@@ -155,10 +156,12 @@ linemap_add (struct line_maps *set, enum
   map->reason = reason;
   map->sysp = sysp;
   map->start_location = start_location;
   map->to_file = to_file;
   map->to_line = to_line;
+  if (linebuf != NULL)
+    map->linebuf = linebuf;
   set->cache = set->used++;
   map->column_bits = 0;
   set->highest_location = start_location;
   set->highest_line = start_location;
   set->max_column_hint = 0;
@@ -181,11 +184,11 @@ linemap_add (struct line_maps *set, enum
   return map;
 }
 
 source_location
 linemap_line_start (struct line_maps *set, linenum_type to_line,
-		    unsigned int max_column_hint)
+		    unsigned int max_column_hint, const unsigned char *linebuf)
 {
   struct line_map *map = &set->maps[set->used - 1];
   source_location highest = set->highest_location;
   source_location r;
   linenum_type last_line = SOURCE_LINE (map, set->highest_line);
@@ -198,10 +201,11 @@ linemap_line_start (struct line_maps *se
     {
       add_map = true;
     }
   else
     max_column_hint = set->max_column_hint;
+
   if (add_map)
     {
       int column_bits;
       if (max_column_hint > 100000 || highest > 0xC0000000)
 	{
@@ -223,11 +227,11 @@ linemap_line_start (struct line_maps *se
 	 single line we can sometimes just increase its column_bits instead. */
       if (line_delta < 0
 	  || last_line != map->to_line
 	  || SOURCE_COLUMN (map, highest) >= (1U << column_bits))
 	map = (struct line_map *) linemap_add (set, LC_RENAME, map->sysp,
-					       map->to_file, to_line);
+					       map->to_file, to_line, linebuf);
       map->column_bits = column_bits;
       r = map->start_location + ((to_line - map->to_line) << column_bits);
     }
   else
     r = highest - SOURCE_COLUMN (map, highest)
@@ -251,11 +255,12 @@ linemap_position_for_column (struct line
 	  return r;
 	}
       else
 	{
 	  struct line_map *map = &set->maps[set->used - 1];
-	  r = linemap_line_start (set, SOURCE_LINE (map, r), to_column + 50);
+	  r = linemap_line_start (set, SOURCE_LINE (map, r), to_column + 50,
+				  SOURCE_LINEBUFFER (map, r));
 	}
     }
   r = r + to_column;
   if (r >= set->highest_location)
     set->highest_location = r;
Index: libcpp/files.c
===================================================================
--- libcpp/files.c	(revision 138935)
+++ libcpp/files.c	(working copy)
@@ -1342,11 +1342,12 @@ _cpp_pop_file_buffer (cpp_reader *pfile,
   /* Invalidate control macros in the #including file.  */
   pfile->mi_valid = false;
 
   if (file->buffer_start)
     {
-      free ((void *) file->buffer_start);
+      /* CARET: We may need to not free this.  */
+      /*      free ((void *) file->buffer_start);*/
       file->buffer_start = NULL;
       file->buffer = NULL;
       file->buffer_valid = false;
     }
 }
Index: libcpp/include/line-map.h
===================================================================
--- libcpp/include/line-map.h	(revision 138935)
+++ libcpp/include/line-map.h	(working copy)
@@ -66,10 +66,12 @@ struct line_map GTY(())
   ENUM_BITFIELD (lc_reason) reason : CHAR_BIT;
   /* The sysp field isn't really needed now that it's in cpp_buffer.  */
   unsigned char sysp;
   /* Number of the low-order source_location bits used for a column number.  */
   unsigned int column_bits : 8;
+  /* The input line.  */
+  const unsigned char * linebuf;
 };
 
 /* A set of chronological line_map structures.  */
 struct line_maps GTY(())
 {
@@ -120,11 +122,11 @@ extern void linemap_check_files_exited (
    most recent linemap_add).   MAX_COLUMN_HINT is the highest column
    number we expect to use in this line (but it does not change
    the highest_location).  */
 
 extern source_location linemap_line_start
-(struct line_maps *set, linenum_type to_line,  unsigned int max_column_hint);
+(struct line_maps *set, linenum_type to_line,  unsigned int max_column_hint, const unsigned char *linebuf);
 
 /* Add a mapping of logical source line to physical source file and
    line number.
 
    The text pointed to by TO_FILE must have a lifetime
@@ -135,11 +137,11 @@ extern source_location linemap_line_star
 
    A call to this function can relocate the previous set of
    maps, so any stored line_map pointers should not be used.  */
 extern const struct line_map *linemap_add
   (struct line_maps *, enum lc_reason, unsigned int sysp,
-   const char *to_file, linenum_type to_line);
+   const char *to_file, linenum_type to_line, const unsigned char *linebuf);
 
 /* Given a logical line, returns the map from which the corresponding
    (source file, line) pair can be deduced.  */
 extern const struct line_map *linemap_lookup
   (struct line_maps *, source_location);
@@ -155,10 +157,30 @@ extern void linemap_print_containing_fil
   ((((LOC) - (MAP)->start_location) >> (MAP)->column_bits) + (MAP)->to_line)
 
 #define SOURCE_COLUMN(MAP, LOC) \
   (((LOC) - (MAP)->start_location) & ((1 << (MAP)->column_bits) - 1))
 
+static inline const unsigned char * 
+SOURCE_LINEBUFFER (const struct line_map * MAP, source_location LOCATION)
+{
+  const unsigned char * linebuf = (MAP)->linebuf;
+  
+  if (linebuf != NULL)
+    {
+      int i = SOURCE_LINE(MAP, LOCATION) - (MAP)->to_line;
+      for (;;) 
+	{
+	  if (i == 0 || *linebuf == '\0')
+	    break;
+	  if (*linebuf == '\n')
+	    i--;
+	  linebuf++;
+	}
+    }
+  return linebuf;
+}
+
 /* Returns the last source line within a map.  This is the (last) line
    of the #include, or other directive, that caused a map change.  */
 #define LAST_SOURCE_LINE(MAP) \
   SOURCE_LINE (MAP, LAST_SOURCE_LINE_LOCATION (MAP))
 #define LAST_SOURCE_LINE_LOCATION(MAP) \
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 138935)
+++ libcpp/internal.h	(working copy)
@@ -67,11 +67,11 @@ struct cset_converter
 
 #define CPP_INCREMENT_LINE(PFILE, COLS_HINT) do { \
     const struct line_maps *line_table = PFILE->line_table; \
     const struct line_map *map = &line_table->maps[line_table->used-1]; \
     linenum_type line = SOURCE_LINE (map, line_table->highest_line); \
-    linemap_line_start (PFILE->line_table, line + 1, COLS_HINT); \
+    linemap_line_start (PFILE->line_table, line + 1, COLS_HINT, PFILE->buffer->cur); \
   } while (0)
 
 /* Maximum nesting of cpp_buffers.  We use a static limit, partly for
    efficiency, and partly to limit runaway recursion.  */
 #define CPP_STACK_MAX 200

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Joseph S. Myers]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 502 bytes --]

On Thu, 14 Aug 2008, Manuel López-Ibáñez wrote:

> You can see many examples of the caret output by configuring with
> --enable-caret-diagnostics, then reverting the changes to
> gcc/testsuite/lib/gcc.exp and running the testsuite. Check the output
> in the gcc.log and g++.log files.

It's clear it should be controlled by a -Wcaret (or similar) option rather 
than a configure time option; the choice of style is a matter of user 
preference.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Manuel López-Ibáñez]

2008/8/14 Joseph S. Myers <joseph@codesourcery.com>:
> On Thu, 14 Aug 2008, Manuel López-Ibáñez wrote:
>
>> You can see many examples of the caret output by configuring with
>> --enable-caret-diagnostics, then reverting the changes to
>> gcc/testsuite/lib/gcc.exp and running the testsuite. Check the output
>> in the gcc.log and g++.log files.
>
> It's clear it should be controlled by a -Wcaret (or similar) option rather
> than a configure time option; the choice of style is a matter of user
> preference.
>

It is controlled by -fdiagnostics-show-caret. See the diff for gcc/opts.c.

The configure options are meant to enable/disable all code related to
caret printing in a similar way as it was done with mapped locations.
This was requested the first time I sent this patch because it was
considered too experimental to have it even with
-fno-diagnostics-show-caret as the default.

Cheers,

Manuel.

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Joseph S. Myers]

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

On Thu, 14 Aug 2008, Manuel López-Ibáñez wrote:

> It is controlled by -fdiagnostics-show-caret. See the diff for gcc/opts.c.
> 
> The configure options are meant to enable/disable all code related to
> caret printing in a similar way as it was done with mapped locations.
> This was requested the first time I sent this patch because it was
> considered too experimental to have it even with
> -fno-diagnostics-show-caret as the default.

Mapped locations were sitting around on trunk for a long time as a 
bitrotten feature; I don't think that's a good example here.  When the 
code works and passes review it should go on trunk, default remaining the 
present diagnostic style according to the GNU Coding Standards, no 
configure option.

I don't think the option should necessarily just be boolean; once choice 
that may make sense would be caret diagnostics for the first diagnostic 
from an input file only, to avoid blowing up the output size when one 
mistake causes a cascade of diagnostics.  (This is a matter of designing 
the option as e.g. -fdiagnostics-show-caret={no,yes,first} rather than as 
-f/fno-, not a matter of needing such a feature implemented in the first 
version going on trunk.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Manuel López-Ibáñez]

2008/8/14 Joseph S. Myers <joseph@codesourcery.com>:
> On Thu, 14 Aug 2008, Manuel López-Ibáñez wrote:
>
>> It is controlled by -fdiagnostics-show-caret. See the diff for gcc/opts.c.
>>
>> The configure options are meant to enable/disable all code related to
>> caret printing in a similar way as it was done with mapped locations.
>> This was requested the first time I sent this patch because it was
>> considered too experimental to have it even with
>> -fno-diagnostics-show-caret as the default.
>
> Mapped locations were sitting around on trunk for a long time as a
> bitrotten feature; I don't think that's a good example here.  When the
> code works and passes review it should go on trunk, default remaining the
> present diagnostic style according to the GNU Coding Standards, no
> configure option.

That is nice to know *after* wasting all that time figuring out how
configure works [*], ;-)

Even if the configure options do not control the compilation of the
code, they should at least control the default. I think that, at least
for development versions, the default should be on. We do want
developers to realise when their new warning/error is pointing to the
wrong location. Moreover, some user-friendly distributions may want to
enable this by default. Therefore, I would argue to keep the configure
options to determine the default value of -fdiagnostics-show-caret.

Cheers,

Manuel.

[*] BTW, thanks to  Basile Starynkevitch  for writing
http://gcc.gnu.org/wiki/Regenerating_GCC_Configuration

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Joseph S. Myers]

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

On Thu, 14 Aug 2008, Manuel López-Ibáñez wrote:

> Even if the configure options do not control the compilation of the
> code, they should at least control the default. I think that, at least

No, whatever the default is there should definitely be no configure option 
to control it.  Configure options controlling such user-visible behavior 
as this are a very bad idea; users should not find the same options, 
makefiles etc. acting differently depending on what distribution of the 
same version of GCC they have.  It would be better to remove some 
configuration options than to add them: make GCC more confident it knows 
the right way to configure various things for each system and configure 
them the right way unconditionally.

> for development versions, the default should be on. We do want
> developers to realise when their new warning/error is pointing to the
> wrong location. Moreover, some user-friendly distributions may want to
> enable this by default. Therefore, I would argue to keep the configure
> options to determine the default value of -fdiagnostics-show-caret.

I argue the default should be output (a) compatible with consumers 
expecting a series of diagnostic lines following the GNU Coding Standards 
and (b) compact according to existing expectations of GCC output (one line 
per message instead of three or more, for front ends that have been doing 
that so far, but probably keeping carets for any front ends that have been 
using them with their own machinery).  Compatibility especially argues for 
keeping the existing format, but my experience is also that the main use 
of a diagnostic is to find the right line in an editor and only on a small 
proportion of occasions (when the warning option could be added to the 
compilation command manually) is it at all unclear what the problem is, 
such that more precise details (typically pointing into preprocessed 
source) would be useful: almost all the time caret diagnostics would make 
the bulk of diagnostic output into unnecessary noise.

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Manuel López-Ibáñez]

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.

But even more importantly. No GCC developer is going to explicitly
enable caret diagnostics while developing GCC. How many nowadays use
-fshow-column or -fdiagnostics-show-option to check locations?

Moreover, caret diagnostics was mentioned as the way to solve the PRs
that Aldy mentioned. If it is disabled by default, how does it solve
anything? Why bother? I would really feel that I contributed to make
GCC worse if GCC diagnostics are less expressive because we have the
excuse of "you could enable the caret". I feel that a lot of PRs that
request for better diagnostics would be closed that way.

I feel that this thread is going again the same way as the ones
before. Someone says: Hey, having caret diagnostics would solve a lot
of problems! Everybody says: Yeah, that would be cool! We could do
this and that and all kinds of cool things. Then someone says: Oh yes
but we need to solve all these boring things that nobody ever really
looks and it should be disabled by default. Then one year later
someone says: Hey, having caret diagnostics would solve a lot of
problems!

Cheers,

Manuel.

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.

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Joseph S. Myers]

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

On Thu, 14 Aug 2008, 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

I do not see how your reply relates to the text you quote about not having 
a configure option, as opposed to the discussion of what the default 
should be.

> read the manual. Neither old users do. New functionality disabled by

I certainly did read the manual (the old "Using and Porting") when I first 
started to use GCC, identified those warning options that seemed good to 
be and put them in the standard set of warning options I use in my 
makefiles, and have revised that set from time to time for new versions 
and absed on experience.

> Moreover, caret diagnostics was mentioned as the way to solve the PRs
> that Aldy mentioned. If it is disabled by default, how does it solve
> anything? Why bother? I would really feel that I contributed to make

The solution is producing accurate location ranges, which can be used (a) 
to print more accurate expressions within the text of diagnostics in the 
existing style, (b) to print GCS-compliant ranges in text that IDEs can 
parse to highlight the relevant text in their editors (and we should 
expect that tools such as GCC and GDB are increasingly going to be used as 
a back end to other tools rather than just directly on the command line by 
users), and (c) for caret diagnostics for users liking those on the 
command line.  Caret diagnostics are only one of the styles in which the 
accurate location information can be used, and implementing an individial 
style is only a small part of the solution.  The PRs are about (a): cases 
where an expression text is displayed within the existing diagnostic text, 
badly.

Naturally all cases covered by (a) should have tests in the testsuite 
checking the right text is printed.  We should also make the testsuite 
able to test location ranges for diagnostics that don't include expression 
text for the relevant range, and then insist in patch review that tests of 
new front-end diagnostics appropriately assert the range involved, as well 
as converting existing tests to more precise assertions over time.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Aldy Hernandez]

> 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.

Why not get the caret diagnostics in, disabled by default, but with
tests that test whatever functionality actually works.  Then fix the
column information so it's accurate, and finally enable caret
diagnostics on by default.  And as Joseph said, when the caret
diagnostics are enabled, they should be displayed in a format that
follow the GNU coding standards for diagnostics.

> that Aldy mentioned. If it is disabled by default, how does it solve

I envision the caret diagnostics being disabled for only a short while--
while we beat some sense into the column information.  There's no point
in attacking everything at once.

Aldy

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 (was: broken FE diagnostics wrt complex expressions)

[Manuel López-Ibáñez]

2008/8/14 Aldy Hernandez <aldyh@redhat.com>:
>
> I envision the caret diagnostics being disabled for only a short while--
> while we beat some sense into the column information.  There's no point
> in attacking everything at once.

Then, I think we are talking past each other. To be crystal clear, my
opinion is:

* In the near future, make -fdiagnostics-show-caret the default at
least while in experimental mode or at least during stages1 and 2.
When making a release -fno-diagnostics-show-caret would be the
default. Do this through a configure option that sets the default.

* In the far away future, review the defaults and get rid of the
configure option.

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

[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]

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

[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

[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

[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

[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 (was: broken FE diagnostics wrt complex expressions)

[Aldy Hernandez]

> * In the near future, make -fdiagnostics-show-caret the default at
> least while in experimental mode or at least during stages1 and 2.
> When making a release -fno-diagnostics-show-caret would be the
> default. Do this through a configure option that sets the default.
> 
> * In the far away future, review the defaults and get rid of the
> configure option.

To be honest, I don't mind either way.  Either your option or Joseph's
is fine.  I am however interested in getting the caret diagnostics in,
and then I can work on location information that will benefit everyone.

Aldy

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

[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:

> 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

[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

[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

[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

[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

[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

[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

[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

[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 (was: broken FE diagnostics wrt complex expressions)

[Gabriel Dos Reis]

On Thu, Aug 14, 2008 at 7:39 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Thu, 14 Aug 2008, Manuel López-Ibáñez wrote:
>
> I don't think the option should necessarily just be boolean; once choice
> that may make sense would be caret diagnostics for the first diagnostic
> from an input file only, to avoid blowing up the output size when one
> mistake causes a cascade of diagnostics.  (This is a matter of designing
> the option as e.g. -fdiagnostics-show-caret={no,yes,first} rather than as
> -f/fno-, not a matter of needing such a feature implemented in the first
> version going on trunk.)

Yes, -diagnostics-show-caret=<value> is more flexible.

>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: [PATCH] caret diagnostics (was: broken FE diagnostics wrt complex expressions)

[Gabriel Dos Reis]

On Thu, Aug 14, 2008 at 12:14 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> * In the near future, make -fdiagnostics-show-caret the default at
>> least while in experimental mode or at least during stages1 and 2.
>> When making a release -fno-diagnostics-show-caret would be the
>> default. Do this through a configure option that sets the default.
>>
>> * In the far away future, review the defaults and get rid of the
>> configure option.
>
> To be honest, I don't mind either way.  Either your option or Joseph's
> is fine.  I am however interested in getting the caret diagnostics in,
> and then I can work on location information that will benefit everyone.

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).

-- Gaby



>
> Aldy
>

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

[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

[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

[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

[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]

> 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