[PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Manuel López-Ibáñez]

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

This patch adds handling of Fortran %C using the common diagnostics
machinery. This is achieved by dynamically generating a location given
a location and an offset. This only works for non-macro line-maps (for
now), but this is OK since Fortran does not have virtual locations
(and I'm afraid it won't have them in the foreseeable future).

Dodji, are the linemap_asserts() appropriate? I tried to follow your
previous comments whenever possible.

The output is now:

gfortran.dg/use_without_only_1.f90:9:6:

   USE foo ! { dg-warning "6:has no ONLY qualifier" }
      1
Warning: USE statement at (1) has no ONLY qualifier [-Wuse-without-only]
gfortran.dg/use_without_only_1.f90:13:9:

      USE foo ! { dg-warning "9:has no ONLY qualifier" }
         1
Warning: USE statement at (1) has no ONLY qualifier [-Wuse-without-only]
gfortran.dg/use_without_only_1.f90:19:9:

      USE ISO_C_BINDING ! { dg-warning "9:has no ONLY qualifier" }
         1
Warning: USE statement at (1) has no ONLY qualifier [-Wuse-without-only]

that is, almost identical to the current output apart from mentioning
the option and the colors. (Spacing is surely messed up, gmail sucks,
sorry).

Strangely, the new warning cannot be parsed by the regex in
gfortran-dg.exp because they got confused by the lack of caret
information (gfc_warning_now_2 obeys -fno-diagnostics-show-caret,
which is the default in the testsuite). Thus I rewrote them in a way
that seems to me more clear and less prone to failure (in particular
being more strict about matching at the start of the line). The new
regex have the benefit that one can test column numbers in testcases
now like the C/C++ FE do.

Of course, this opens the way to convert a large number of
warning/errors in Fortran to the common diagnostics machinery, but
before doing that I wanted to get the basic infrastructure right.

Boot&regtested on x86_64-linux-gnu.

OK?

libcpp/ChangeLog:

2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    PR fortran/44054
    * include/line-map.h (linemap_position_for_loc_and_offset):
    Declare.
    * line-map.c (linemap_position_for_loc_and_offset): New.


gcc/fortran/ChangeLog:

2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    PR fortran/44054
    * gfortran.h (warn_use_without_only): Remove.
    (gfc_diagnostics_finish): Declare.
    * error.c: Include tree-diagnostics.h
    (gfc_format_decoder): New.
    (gfc_diagnostics_init): Use gfc_format_decoder. Set default caret
    char.
    (gfc_diagnostics_finish): Restore tree diagnostics defaults, but
    keep gfc_diagnostics_starter and finalizer. Restore default caret.
    * options.c: Remove all uses of warn_use_without_only.
    * lang.opt (Wuse-without-only): Add Var.
    * module.c (gfc_use_module): Use gfc_warning_now_2.
    * f95-lang.c (gfc_be_parse_file): Call gfc_diagnostics_finish.

gcc/testsuite/ChangeLog:

2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    PR fortran/44054
    * lib/gfortran-dg.exp: Update regexp to match locus and message
    without caret.
    * gfortran.dg/use_without_only_1.f90: Add column numbers.

[-- Attachment #2: fortran-diagnostics-5.diff --]
[-- Type: text/plain, Size: 11194 bytes --]

diff -u gcc/fortran/gfortran.h gcc/fortran/gfortran.h
--- gcc/fortran/gfortran.h	(working copy)
+++ gcc/fortran/gfortran.h	(working copy)
@@ -2455,7 +2455,6 @@
   int warn_tabs;
   int warn_underflow;
   int warn_intrinsic_shadow;
-  int warn_use_without_only;
   int warn_intrinsics_std;
   int warn_character_truncation;
   int warn_array_temp;
@@ -2691,7 +2690,8 @@
 } gfc_error_buf;
 
 void gfc_error_init_1 (void);
-void gfc_diagnostics_init(void);
+void gfc_diagnostics_init (void);
+void gfc_diagnostics_finish (void);
 void gfc_buffer_error (int);
 
 const char *gfc_print_wide_char (gfc_char_t);
diff -u gcc/fortran/error.c gcc/fortran/error.c
--- gcc/fortran/error.c	(working copy)
+++ gcc/fortran/error.c	(working copy)
@@ -40,6 +40,7 @@
 
 #include "diagnostic.h"
 #include "diagnostic-color.h"
+#include "tree-diagnostic.h" /* tree_diagnostics_defaults */
 
 static int suppress_errors = 0;
 
@@ -958,6 +959,38 @@
   buffer_flag = i;
 }
 
+/* Called from output_format -- during diagnostic message processing --
+   to handle Fortran specific format specifiers with the following meanings:
+
+   %C  Current locus (no argument) 
+*/
+static bool
+gfc_format_decoder (pretty_printer *pp,
+		    text_info *text, const char *spec,
+		    int precision ATTRIBUTE_UNUSED, bool wide ATTRIBUTE_UNUSED,
+		    bool plus ATTRIBUTE_UNUSED, bool hash ATTRIBUTE_UNUSED)
+{
+  switch (*spec)
+    {
+    case 'C':
+      {
+	static const char *result = "(1)"; 
+	gcc_assert (gfc_current_locus.nextc - gfc_current_locus.lb->line >= 0);
+        unsigned int c1 = gfc_current_locus.nextc - gfc_current_locus.lb->line;
+        gcc_assert (text->locus);
+	*text->locus 
+	  = linemap_position_for_loc_and_offset (line_table,
+						 gfc_current_locus.lb->location,
+						 c1);
+        global_dc->caret_char = '1';
+	pp_string (pp, result);
+	return true;
+      }
+    default:
+      return false;
+    }
+}
+
 /* Return a malloc'd string describing a location.  The caller is
    responsible for freeing the memory.  */
 static char *
@@ -1357,4 +1390,16 @@
   diagnostic_starter (global_dc) = gfc_diagnostic_starter;
   diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
+  diagnostic_format_decoder (global_dc) = gfc_format_decoder;
+  global_dc->caret_char = '^';
+}
+
+void
+gfc_diagnostics_finish (void)
+{
+  tree_diagnostics_defaults (global_dc);
+  /* We still want to use the gfc starter and finalizer, not the tree
+     defaults.  */
+  diagnostic_starter (global_dc) = gfc_diagnostic_starter;
+  diagnostic_finalizer (global_dc) = gfc_diagnostic_finalizer;
   global_dc->caret_char = '^';
 }
diff -u gcc/fortran/options.c gcc/fortran/options.c
--- gcc/fortran/options.c	(working copy)
+++ gcc/fortran/options.c	(working copy)
@@ -107,7 +107,6 @@
   gfc_option.warn_tabs = 1;
   gfc_option.warn_underflow = 1;
   gfc_option.warn_intrinsic_shadow = 0;
-  gfc_option.warn_use_without_only = 0;
   gfc_option.warn_intrinsics_std = 0;
   gfc_option.warn_align_commons = 1;
   gfc_option.warn_real_q_constant = 0;
@@ -737,10 +736,6 @@
       gfc_option.warn_intrinsic_shadow = value;
       break;
 
-    case OPT_Wuse_without_only:
-      gfc_option.warn_use_without_only = value;
-      break;
-
     case OPT_Walign_commons:
       gfc_option.warn_align_commons = value;
       break;
only in patch2:
unchanged:
--- gcc/testsuite/lib/gfortran-dg.exp	(revision 216257)
+++ gcc/testsuite/lib/gfortran-dg.exp	(working copy)
@@ -47,38 +47,49 @@ proc gfortran-dg-test { prog do_what ext
     #
     #       some code and some more code
     #              1       2
     #     Error: Some error at (1) and (2)
     #
-    # Where [locus] is either [line] or [line].[columns] .
+    # or
+    #     [name]:[locus]: Error: Some error
+    #
+    # Where [locus] is either [line] or [line].[column] or
+    # [line].[column]-[column] .
     #
     # We collapse these to look like:
     #  [name]:[line]:[column]: Error: Some error at (1) and (2)
     # or
     #  [name]:[line]:[column]: Error: Some error at (1) and (2)
     #  [name]:[line2]:[column]: Error: Some error at (1) and (2)
-    # We proceed in two steps: first we deal with the form with two
-    # different locus lines, then with the form with only one locus line.
     #
     # Note that these regexps only make sense in the combinations used below.
     # Note also that is imperative that we first deal with the form with
     # two loci.
-    set locus_regexp "(\[^\n\]*):(\[0-9\]+)\[\.:\](\[0-9\]*)(-\[0-9\]*)?:\n\n\[^\n\]*\n\[^\n\]*\n"
-    set diag_regexp "(\[^\n\]*)\n"
+    set locus_regexp "(\[^\n\]+:\[0-9\]+)\[\.:\](\[0-9\]+)(-\[0-9\]+)?:\n\n\[^\n\]+\n\[^\n\]+\n"
+    set diag_regexp "(\[^\n\]+)\n"
 
-    # Add column number if none exists
-    set colnum_regexp "(Warning: |Error: )?(\[^\n\]*):(\[0-9\]+):(\[ \n\])"
-    regsub -all $colnum_regexp $comp_output "\\2:\\3:0:\\4\\1" comp_output
-
-    set two_loci "$locus_regexp$locus_regexp$diag_regexp"
-    set single_locus "$locus_regexp$diag_regexp"
-    regsub -all $two_loci $comp_output "\\1:\\2:\\3: \\9\n\\5:\\6:\\7: \\9\n" comp_output
-    regsub -all $single_locus $comp_output "\\1:\\2:\\3: \\5\n" comp_output
+    # We proceed in steps:
 
-    # Add a line number if none exists
-    regsub -all "(^|\n)(Warning: |Error: )" $comp_output "\\1:0:0: \\2" comp_output
+    # 1. We add first a column number if none exists.
+    # (Some Fortran diagnostics have the locus after Warning|Error)
+    set colnum_regexp "(^|\n)(Warning: |Error: )?(\[^:\n\]+:\[0-9\]+):(\[ \n\])"
+    regsub -all $colnum_regexp $comp_output "\\1\\3:0:\\4\\2" comp_output
+    verbose "comput_output0:\n$comp_output"
 
+    # 2. We deal with the form with two different locus lines,
+    set two_loci "(^|\n)$locus_regexp$locus_regexp$diag_regexp"
+    regsub -all $two_loci $comp_output "\\1\\2:\\3: \\8\n\\5\:\\6: \\8\n" comp_output
+    verbose "comput_output1:\n$comp_output"
+
+    # 3. then with the form with only one locus line.
+    set single_locus "(^|\n)$locus_regexp$diag_regexp"
+    regsub -all $single_locus $comp_output "\\1\\2:\\3: \\5\n" comp_output
+    verbose "comput_output2:\n$comp_output"
+
+    # 4. Add a line number if none exists
+    regsub -all "(^|\n)(Warning: |Error: )" $comp_output "\\1:0:0: \\2" comp_output
+    verbose "comput_output3:\n$comp_output"
     return [list $comp_output $output_file]
 }
 
 proc gfortran-dg-prune { system text } {
     return [gcc-dg-prune $system $text]
only in patch2:
unchanged:
--- gcc/testsuite/gfortran.dg/use_without_only_1.f90	(revision 216257)
+++ gcc/testsuite/gfortran.dg/use_without_only_1.f90	(working copy)
@@ -4,19 +4,19 @@
 MODULE foo
   INTEGER :: bar
 END MODULE
 
 MODULE testmod
-  USE foo ! { dg-warning "has no ONLY qualifier" }
+  USE foo ! { dg-warning "6:has no ONLY qualifier" }
   IMPLICIT NONE
 CONTAINS
   SUBROUTINE S1
-     USE foo ! { dg-warning "has no ONLY qualifier" }
+     USE foo ! { dg-warning "9:has no ONLY qualifier" }
   END SUBROUTINE S1
   SUBROUTINE S2
      USE foo, ONLY: bar 
   END SUBROUTINE
   SUBROUTINE S3
-     USE ISO_C_BINDING ! { dg-warning "has no ONLY qualifier" }
+     USE ISO_C_BINDING ! { dg-warning "9:has no ONLY qualifier" }
   END SUBROUTINE S3
 END MODULE
 ! { dg-final { cleanup-modules "foo testmod" } }
only in patch2:
unchanged:
--- gcc/fortran/lang.opt	(revision 216257)
+++ gcc/fortran/lang.opt	(working copy)
@@ -260,11 +260,11 @@ Warn on intrinsics not part of the selec
 Wmissing-include-dirs
 Fortran
 ; Documented in C/C++
 
 Wuse-without-only
-Fortran Warning
+Fortran Var(warn_use_without_only) Warning
 Warn about USE statements that have no ONLY qualifier
 
 Wopenmp-simd
 Fortran
 ; Documented in C
only in patch2:
unchanged:
--- gcc/fortran/module.c	(revision 216257)
+++ gcc/fortran/module.c	(working copy)
@@ -6742,12 +6742,13 @@ gfc_use_module (gfc_use_list *module)
   module_name = module->module_name;
   gfc_rename_list = module->rename;
   only_flag = module->only_flag;
   current_intmod = INTMOD_NONE;
 
-  if (!only_flag && gfc_option.warn_use_without_only) 
-    gfc_warning_now ("USE statement at %C has no ONLY qualifier");
+  if (!only_flag)
+    gfc_warning_now_2 (OPT_Wuse_without_only,
+		       "USE statement at %C has no ONLY qualifier");
 
   filename = XALLOCAVEC (char, strlen (module_name) + strlen (MODULE_EXTENSION)
 			       + 1);
   strcpy (filename, module_name);
   strcat (filename, MODULE_EXTENSION);
only in patch2:
unchanged:
--- gcc/fortran/f95-lang.c	(revision 216257)
+++ gcc/fortran/f95-lang.c	(working copy)
@@ -218,10 +218,14 @@ gfc_be_parse_file (void)
   warningcount += warnings;
 
   /* Clear the binding level stack.  */
   while (!global_bindings_p ())
     poplevel (0, 0);
+
+  /* Switch to the default tree diagnostics here, because there may be
+     diagnostics before gfc_finish().  */
+  gfc_diagnostics_finish ();
 }
 
 
 /* Initialize everything.  */
 
only in patch2:
unchanged:
--- libcpp/include/line-map.h	(revision 216257)
+++ libcpp/include/line-map.h	(working copy)
@@ -601,10 +601,17 @@ linemap_position_for_column (struct line
    column.  */
 source_location
 linemap_position_for_line_and_column (const struct line_map *,
 				      linenum_type, unsigned int);
 
+/* Encode and return a source_location starting from location LOC
+   and shifting it by OFFSET columns.  */
+source_location
+linemap_position_for_loc_and_offset (struct line_maps *set,
+				     source_location loc,
+				     unsigned int offset);
+
 /* Return the file this map is for.  */
 #define LINEMAP_FILE(MAP)					\
   (linemap_check_ordinary (MAP)->d.ordinary.to_file)
 
 /* Return the line number this map started encoding location from.  */
only in patch2:
unchanged:
--- libcpp/line-map.c	(revision 216257)
+++ libcpp/line-map.c	(working copy)
@@ -631,10 +631,41 @@ linemap_position_for_line_and_column (co
 	  + ((line - ORDINARY_MAP_STARTING_LINE_NUMBER (map))
 	     << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map))
 	  + (column & ((1 << ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map)) - 1)));
 }
 
+/* Encode and return a source_location starting from location LOC
+   and shifting it by OFFSET columns.  */
+
+source_location
+linemap_position_for_loc_and_offset (struct line_maps *set,
+				     source_location loc,
+				     unsigned int offset)
+{
+  const struct line_map * map = NULL;
+
+  /* This function does not support virtual locations yet.  */
+  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
+
+  if (offset == 0)
+    return loc;
+
+  /* First, we find the real location and shift it.  */
+  loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map);
+  linemap_assert (MAP_START_LOCATION (map) < loc + offset);
+
+  offset += SOURCE_COLUMN (map, loc);
+  linemap_assert (offset < (1u << map->d.ordinary.column_bits));
+
+  source_location r = 
+    linemap_position_for_line_and_column (map,
+					  SOURCE_LINE (map, loc),
+					  offset);
+  linemap_assert (map == linemap_lookup (set, r));
+  return r;
+}
+
 /* Given a virtual source location yielded by a map (either an
    ordinary or a macro map), returns that map.  */
 
 const struct line_map*
 linemap_lookup (struct line_maps *set, source_location line)

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Tobias Burnus]

Manuel López-Ibáñez wrote:
> This patch adds handling of Fortran %C using the common diagnostics
> machinery. This is achieved by dynamically generating a location given
> a location and an offset. This only works for non-macro line-maps (for
> now), but this is OK since Fortran does not have virtual locations
> (and I'm afraid it won't have them in the foreseeable future).
>
> Dodji, are the linemap_asserts() appropriate? I tried to follow your
> previous comments whenever possible.

> Boot&regtested on x86_64-linux-gnu.
> OK?

 From my side, the patch is OK. Thanks again for your diagnostic work 
(for this patch set and in general)!

Tobias

> libcpp/ChangeLog:
>
> 2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>
>      PR fortran/44054
>      * include/line-map.h (linemap_position_for_loc_and_offset):
>      Declare.
>      * line-map.c (linemap_position_for_loc_and_offset): New.
>
>
> gcc/fortran/ChangeLog:
>
> 2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>
>      PR fortran/44054
>      * gfortran.h (warn_use_without_only): Remove.
>      (gfc_diagnostics_finish): Declare.
>      * error.c: Include tree-diagnostics.h
>      (gfc_format_decoder): New.
>      (gfc_diagnostics_init): Use gfc_format_decoder. Set default caret
>      char.
>      (gfc_diagnostics_finish): Restore tree diagnostics defaults, but
>      keep gfc_diagnostics_starter and finalizer. Restore default caret.
>      * options.c: Remove all uses of warn_use_without_only.
>      * lang.opt (Wuse-without-only): Add Var.
>      * module.c (gfc_use_module): Use gfc_warning_now_2.
>      * f95-lang.c (gfc_be_parse_file): Call gfc_diagnostics_finish.
>
> gcc/testsuite/ChangeLog:
>
> 2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>
>      PR fortran/44054
>      * lib/gfortran-dg.exp: Update regexp to match locus and message
>      without caret.
>      * gfortran.dg/use_without_only_1.f90: Add column numbers.

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Dodji Seketeli]

Hello Manuel,

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


> Dodji, are the linemap_asserts() appropriate?

Yes they are.  I have some additional comments though.

> libcpp/ChangeLog:
>
> 2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>
>     PR fortran/44054
>     * include/line-map.h (linemap_position_for_loc_and_offset):
>     Declare.
>     * line-map.c (linemap_position_for_loc_and_offset): New.
[...]

> --- libcpp/include/line-map.h	(revision 216257)
> +++ libcpp/include/line-map.h	(working copy)
> @@ -601,10 +601,17 @@ linemap_position_for_column (struct line
>     column.  */
>  source_location
>  linemap_position_for_line_and_column (const struct line_map *,
>  				      linenum_type, unsigned int);
>  
> +/* Encode and return a source_location starting from location LOC
> +   and shifting it by OFFSET columns.  */
> +source_location
> +linemap_position_for_loc_and_offset (struct line_maps *set,
> +				     source_location loc,
> +				     unsigned int offset);
> +

OK.

[...]

> --- libcpp/line-map.c	(revision 216257)
> +++ libcpp/line-map.c	(working copy)

[...]

> +/* Encode and return a source_location starting from location LOC
> +   and shifting it by OFFSET columns.  */
> +

The comment is OK.  I would just add that this function currently only
works with non-virtual locations.

> +source_location
> +linemap_position_for_loc_and_offset (struct line_maps *set,
> +				     source_location loc,
> +				     unsigned int offset)
> +{
> +  const struct line_map * map = NULL;
> +
> +  /* This function does not support virtual locations yet.  */
> +  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
> +
> +  if (offset == 0)
> +    return loc;

Here, I'd replace the above condition and return status statement with:

    if (offset == 0
        /* Adding an offset to a reserved location (like
           UNKNOWN_LOCATION for the C/C++ FEs) does not really make
           sense.  So let's live the location intact in that case.  */
        || loc < RESERVED_LOCATION)
      return loc;

> +
> +  /* First, we find the real location and shift it.  */
> +  loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map);
> +  linemap_assert (MAP_START_LOCATION (map) < loc + offset);

OK.

First I'd add a comment above the assert that says:

/* The new location (loc + offset) should be higher than the first
   location encoded by MAP.  */

and I'd add another assert:

    /* If MAP is not the last line map of its set, then the new location
       (loc + offset) should be less than the first location encoded by
       the next line map of the set.  */
    if (map < LINEMAPS_LAST_ORDINARY_MAP(set))
      linemap_assert(MAP_START_LOCATION(&map[1]) < loc + offset);

> +
> +  offset += SOURCE_COLUMN (map, loc);
> +  linemap_assert (offset < (1u << map->d.ordinary.column_bits));
> +
> +  source_location r = 
> +    linemap_position_for_line_and_column (map,
> +					  SOURCE_LINE (map, loc),
> +					  offset);
> +  linemap_assert (map == linemap_lookup (set, r));
> +  return r;
> +}
> +

OK.

So the line map part of the patch is OK from me if it passes bootstrap
with the added asserts.

Thank you for looking into this.

Cheers.

-- 
		Dodji

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Dodji Seketeli]

Sorry, I forgot to make it clear that I have no power to approve the
line-map changes.  I just gave my casual point view; so please take it
for what it is worth.

I am CC-ing Tom and Jason who can approve this.

Cheers.

Dodji Seketeli <dodji@redhat.com> writes:

> Hello Manuel,
>
> Manuel López-Ibáñez <lopezibanez@gmail.com> writes:
>
>
>> Dodji, are the linemap_asserts() appropriate?
>
> Yes they are.  I have some additional comments though.
>
>> libcpp/ChangeLog:
>>
>> 2014-10-16  Manuel López-Ibáñez  <manu@gcc.gnu.org>
>>
>>     PR fortran/44054
>>     * include/line-map.h (linemap_position_for_loc_and_offset):
>>     Declare.
>>     * line-map.c (linemap_position_for_loc_and_offset): New.
> [...]
>
>> --- libcpp/include/line-map.h	(revision 216257)
>> +++ libcpp/include/line-map.h	(working copy)
>> @@ -601,10 +601,17 @@ linemap_position_for_column (struct line
>>     column.  */
>>  source_location
>>  linemap_position_for_line_and_column (const struct line_map *,
>>  				      linenum_type, unsigned int);
>>  
>> +/* Encode and return a source_location starting from location LOC
>> +   and shifting it by OFFSET columns.  */
>> +source_location
>> +linemap_position_for_loc_and_offset (struct line_maps *set,
>> +				     source_location loc,
>> +				     unsigned int offset);
>> +
>
> OK.
>
> [...]
>
>> --- libcpp/line-map.c	(revision 216257)
>> +++ libcpp/line-map.c	(working copy)
>
> [...]
>
>> +/* Encode and return a source_location starting from location LOC
>> +   and shifting it by OFFSET columns.  */
>> +
>
> The comment is OK.  I would just add that this function currently only
> works with non-virtual locations.
>
>> +source_location
>> +linemap_position_for_loc_and_offset (struct line_maps *set,
>> +				     source_location loc,
>> +				     unsigned int offset)
>> +{
>> +  const struct line_map * map = NULL;
>> +
>> +  /* This function does not support virtual locations yet.  */
>> +  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
>> +
>> +  if (offset == 0)
>> +    return loc;
>
> Here, I'd replace the above condition and return status statement with:
>
>     if (offset == 0
>         /* Adding an offset to a reserved location (like
>            UNKNOWN_LOCATION for the C/C++ FEs) does not really make
>            sense.  So let's live the location intact in that case.  */
>         || loc < RESERVED_LOCATION)
>       return loc;
>
>> +
>> +  /* First, we find the real location and shift it.  */
>> +  loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map);
>> +  linemap_assert (MAP_START_LOCATION (map) < loc + offset);
>
> OK.
>
> First I'd add a comment above the assert that says:
>
> /* The new location (loc + offset) should be higher than the first
>    location encoded by MAP.  */
>
> and I'd add another assert:
>
>     /* If MAP is not the last line map of its set, then the new location
>        (loc + offset) should be less than the first location encoded by
>        the next line map of the set.  */
>     if (map < LINEMAPS_LAST_ORDINARY_MAP(set))
>       linemap_assert(MAP_START_LOCATION(&map[1]) < loc + offset);
>
>> +
>> +  offset += SOURCE_COLUMN (map, loc);
>> +  linemap_assert (offset < (1u << map->d.ordinary.column_bits));
>> +
>> +  source_location r = 
>> +    linemap_position_for_line_and_column (map,
>> +					  SOURCE_LINE (map, loc),
>> +					  offset);
>> +  linemap_assert (map == linemap_lookup (set, r));
>> +  return r;
>> +}
>> +
>
> OK.
>
> So the line map part of the patch is OK from me if it passes bootstrap
> with the added asserts.
>
> Thank you for looking into this.
>
> Cheers.

-- 
		Dodji

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Tobias Burnus]

Hi Manuel,

Dodji Seketeli wrote:
> Sorry, I forgot to make it clear that I have no power to approve the
> line-map changes.  I just gave my casual point view; so please take it
> for what it is worth.
>
> I am CC-ing Tom and Jason who can approve this.

And I have a minor nit, which I forgot in my original review - even 
though I had spotted it before: In gfc_format_decoder, you are mixing 
space and tab indention.

Tobias

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Manuel López-Ibáñez]

On 23 October 2014 21:10, Dodji Seketeli <dodji@redhat.com> wrote:
> Sorry, I forgot to make it clear that I have no power to approve the
> line-map changes.  I just gave my casual point view; so please take it
> for what it is worth.
>
> I am CC-ing Tom and Jason who can approve this.

I don't see the CCs in the copy I received. Please, Tom and Jason,
could you nominate Dodji as line-map.[ch] and input.[ch] maintainer?
He is surely the person who knows more about it right now. Or simply
state that line-map.[ch] and input.[ch] fall within the diagnostic
machinery.

Cheers,

Manuel.

Re: [PATCH diagnostics/fortran] dynamically generate locations from offset + handle %C

[Manuel López-Ibáñez]

Hi Dodji,

On 23 October 2014 at 13:11, Dodji Seketeli <dodji@redhat.com> wrote:
>     /* If MAP is not the last line map of its set, then the new location
>        (loc + offset) should be less than the first location encoded by
>        the next line map of the set.  */

which is currently implemented as:

if (map != LINEMAPS_LAST_ORDINARY_MAP (set))
    linemap_assert (loc + offset < MAP_START_LOCATION (&map[1]));

However, this can easily trigger in Fortran, for example, when adding
one line adds a new map, the location of the new map is 1 +
set->highest_location. If the highest_location corresponds to the
start of the previous line (because Fortran does not actually track
columns yet with line-maps) then any offset will trigger this.

One solution could be to make the new map's start location =
set->highest_location + set->max_column_hint, but I'm not sure if
there is a more conservative approach.

I'm going to propose a patch that changes the asserts to assert only
if checking otherwise return loc unchanged. That seems safer than
giving a wildly inaccurate location.

Cheers,

Manuel.