Re: [PATCH] PR preprocessor/42014

[Manuel López-Ibáñez <lopezibanez at gmail dot com>]

> 2014-10-18 23:07 GMT+02:00 Krzesimir Nowak <qdlacz@gmail.com>:
>> Hello.
>>
>> This is my first patch for GCC. I already started a paperwork for
>> copyright assignment (sent an email to fsf-records at gnu org) -
>> waiting for response.
>>
>> So, about this patch - it basically removes column printing from "In
>> file included from ..." lines, as the column information always
>> returned 0. Not sure if this is correct assumption - I tested only C
>> and C++, so I don't know if other frontends (ada, go?) provide column
>> information for include lines. Anyway, column information here is
>> probably not useful.
>>
>> Or maybe it is, if GCC supports some language with include syntax like
>> followish:
>> #include <header_1.h>, <header_2.h>, <header_3.h>
>>
>> Maybe in this case printing column number has sense?
>>
>> I need help with testcase - I don't know how to implement it
>> correctly. The output of compilation is something like this:
>>
>> In file included from .../pr42014-2.h:2,
>>                  from .../pr42014-1.h:3,
>>                  from .../pr42014.c:4:
>> .../pr42014-3.h:1:7: error: 'foo' was not declared in this scope
>>
>> How to check the "from" lines? Is there some dg-foo (dg-grep?) command
>> for it? dg-excess-errors is likely not suited for this purpose.
>
> I suppose I will have to add a preprocessed file and try using dg-message.

Hi Krzesimir,

I think you are overcomplicating it. The original reporter complained
simply that there is an inconsistency between the first line and the
next ones when -fshow-column is enabled (which is now the default but
it wasn't some years ago). The following patch is sufficient to fix
this inconsistency:

Index: diagnostic.c
===================================================================
--- diagnostic.c    (revision 216462)
+++ diagnostic.c    (working copy)
@@ -528,8 +528,8 @@
       if (context->show_column)
         pp_verbatim (context->printer,
              "In file included from %r%s:%d:%d%R", "locus",
-             LINEMAP_FILE (map),
-             LAST_SOURCE_LINE (map), LAST_SOURCE_COLUMN (map));
+             LINEMAP_FILE (map), LAST_SOURCE_LINE (map),
+             LAST_SOURCE_COLUMN (map));
       else
         pp_verbatim (context->printer,
              "In file included from %r%s:%d%R", "locus",
@@ -537,9 +537,15 @@
       while (! MAIN_FILE_P (map))
         {
           map = INCLUDED_FROM (line_table, map);
-          pp_verbatim (context->printer,
-               ",\n                 from %r%s:%d%R", "locus",
-               LINEMAP_FILE (map), LAST_SOURCE_LINE (map));
+          if (context->show_column)
+        pp_verbatim (context->printer,
+                 ",\n                 from %r%s:%d:%d%R", "locus",
+                 LINEMAP_FILE (map), LAST_SOURCE_LINE (map),
+                 LAST_SOURCE_COLUMN (map));
+          else
+        pp_verbatim (context->printer,
+                 ",\n                 from %r%s:%d%R", "locus",
+                 LINEMAP_FILE (map), LAST_SOURCE_LINE (map));
         }
       pp_verbatim (context->printer, ":");
       pp_newline (context->printer);

You can test this by simply building gcc and using -fshow-column vs.
-fno-show-column. I think a testsuite testcase will be hard to build
because DejaGNU. It doesn't seem worth the effort for such a minor
issue. Given that you seem to have enough knowledge and ability to
modify GCC and submit good patches, it would be better to spend your
time on more important bugs.

For example, this one needs to be analyzed, we don't even know how it happens:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52998

Or this one, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45333
which I think is just a matter of adding (or factoring out) some of
the logic from maybe_unwind_expanded_macro_loc() and use it in various
places in cp/error.c (print_instantiation_*).

If you are not motivated by those, I can offer more suggestions...

Cheers,

Manuel.