[CCing Rainer and Mike for the gcc-dg.exp part]
On Thu, 2017-10-26 at 07:33 -0400, Nathan Sidwell wrote:
> On the modules branch, I'm starting to add location
> information. Line
> numbers don't really make sense when reporting errors reading a
> binary
> file, so I wanted to change the diagnostics such that line number
> zero
> (which is not a line) is not printed -- one just gets the file
> name. I
> then noticed that we don't elide column zero (also, not a column
> outside
> of emacsland).
>
> This patch changes the diagnostics, such that line-zero prints
> neither
> line nor column and column-zero doesn't print the column.
>
> The testsuite presumes that all diagnostics have a column (which may
> or
> may not be specified in the test pattern). This patch augments it
> such
> that a prefix of '-:' indicates 'no column'. We still default to
> expecting a column
>
> The vast bulk is annotating C & C++ tests that do not have a column.
> Some of those were explicitly checking for column-zero, but many
> just
> expected some arbitrary column number, which happened to be
> zero. Of
> course many (most?) of these diagnostics could be improved to provide
> a
> column. Most are from the preprocessor.
>
> While this is a change in the compiler's output, it's effectively
> returning to a pre-column formatting for the cases where the column
> number is not known. I'd expect (hope?) error message parsers to be
> robust in that case. (I've found it confusing when column-zero is
> printed, as I think columns might be zero-based after all.)
>
> bootstrapped on all languages.
>
> ok?
>
> nathan
Indeed, gcc uses 1-based columns, with 0 meaning "the whole line",
whereas Emacs uses 0-based columns (see the comment in line-map.h).
Probably best to not print them, to avoid confusing the user.
Alternate idea: could show_column become a tri-state:
* default: show non-zero columns
* never: never show columns
* always: always show a column, printing 0 for the no-column case
and then use "always" in our testsuite
?
-fno-show-column would presumably then be a legacy alias for the
"never" value.
> Index: gcc/diagnostic.c
> ===================================================================
> --- gcc/diagnostic.c (revision 254060)
> +++ gcc/diagnostic.c (working copy)
> @@ -293,6 +293,24 @@ diagnostic_get_color_for_kind (diagnosti
> return diagnostic_kind_color[kind];
> }
>
> +/* Return a formatted line and column ':%line:%column'. Elided if
> + zero. The result is a statically allocated buffer. */
> +static const char *
> +maybe_line_and_column (int line, int col)
> +{
> + static char result[32];
> +
> + if (line)
> + {
> + size_t l = sprintf (result, col ? ":%d:%d" : ":%d", line, col);
Possibly a silly question, but is it OK to have a formatted string
call in which some of the arguments aren't consumed? (here "col" is only
consumed for the true case, which consumes 2 arguments; it's not consumed
for the false case).
> + gcc_checking_assert (l + 1 < sizeof (result));
Would snprintf be safer?
Please create a selftest for the function, covering these cases:
* line == 0
* line > 0 and col == 0
* line > 0 and col > 0 (checking output for these cases)
* line == INT_MAX and col == INT_MAX (without checking output, just to tickle
the assert)
* line == INT_MIN and col == INT_MIN (likewise)
Alternatively, please create a selftest for diagnostic_get_location_text,
testing the cases of:
* context->show_column true and false
* N_("<built-in>")
* the above line/col value combos
> + }
> + else
> + result[0] = 0;
> + return result;
> +}
> +
> /* Return a malloc'd string describing a location e.g. "foo.c:42:10".
> The caller is responsible for freeing the memory. */
>
> @@ -303,19 +321,13 @@ diagnostic_get_location_text (diagnostic
> pretty_printer *pp = context->printer;
> const char *locus_cs = colorize_start (pp_show_color (pp), "locus");
> const char *locus_ce = colorize_stop (pp_show_color (pp));
> -
> - if (s.file == NULL)
> - return build_message_string ("%s%s:%s", locus_cs, progname, locus_ce);
> -
> - if (!strcmp (s.file, N_("<built-in>")))
> - return build_message_string ("%s%s:%s", locus_cs, s.file, locus_ce);
> -
> - if (context->show_column)
> - return build_message_string ("%s%s:%d:%d:%s", locus_cs, s.file, s.line,
> - s.column, locus_ce);
> - else
> - return build_message_string ("%s%s:%d:%s", locus_cs, s.file, s.line,
> - locus_ce);
> + const char *file = s.file ? s.file : progname;
> + int line = strcmp (file, N_("<built-in>")) ? s.line : 0;
> + int col = context->show_column ? s.column : 0;
> +
> + const char *line_col = maybe_line_and_column (line, col);
> + return build_message_string ("%s%s%s:%s", locus_cs, file,
> + line_col, locus_ce);
> }
>
> /* Return a malloc'd string describing a location and the severity of the
> @@ -577,21 +589,20 @@ diagnostic_report_current_module (diagno
> if (! MAIN_FILE_P (map))
> {
> map = INCLUDED_FROM (line_table, map);
> - 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));
> - else
> - pp_verbatim (context->printer,
> - "In file included from %r%s:%d%R", "locus",
> - LINEMAP_FILE (map), LAST_SOURCE_LINE (map));
> + const char *line_col
> + = maybe_line_and_column (LAST_SOURCE_LINE (map),
> + context->show_column
> + ? LAST_SOURCE_COLUMN (map) : 0);
> + pp_verbatim (context->printer,
> + "In file included from %r%s%s%R", "locus",
> + LINEMAP_FILE (map), line_col);
> while (! MAIN_FILE_P (map))
> {
> map = INCLUDED_FROM (line_table, map);
> + line_col = maybe_line_and_column (LAST_SOURCE_LINE (map), 0);
> pp_verbatim (context->printer,
> - ",\n from %r%s:%d%R", "locus",
> - LINEMAP_FILE (map), LAST_SOURCE_LINE (map));
> + ",\n from %r%s%s%R", "locus",
> + LINEMAP_FILE (map), line_col);
> }
> pp_verbatim (context->printer, ":");
> pp_newline (context->printer);
[...snip lots of additions of "-:" prefixes to dg- directives...]
There are some testcases where we deliberately don't have a *line*
number; what happens to these?
See e.g. gcc.dg/spellcheck-options-*.c
e.g. gcc.dg/spellcheck-options-1.c, which is:
/* { dg-do compile } */
/* { dg-options "-Wcoercion" } */
/* { dg-error "unrecognized command line option '-Wcoercion'; did you mean
'-Wconversion'?" "" { target *-*-* } 0 } */
> Index: gcc/testsuite/lib/gcc-dg.exp
> ===================================================================
> --- gcc/testsuite/lib/gcc-dg.exp (revision 254060)
> +++ gcc/testsuite/lib/gcc-dg.exp (working copy)
> @@ -1092,24 +1092,27 @@ proc process-message { msgproc msgprefix
> set newentry [lindex ${dg-messages} end]
> set expmsg [lindex $newentry 2]
>
> + set column ""
> # Handle column numbers from the specified expression (if there is
> # one) and set up the search expression that will be used by DejaGnu.
> - if [regexp "^(\[0-9\]+):" $expmsg "" column] {
> + if [regexp {^-:} $expmsg] {
> + # The expected column is -, so shouldn't appear.
> + set expmsg [string range $expmsg 2 end]
> + } elseif [regexp {^[0-9]+:} $expmsg column] {
> # The expression in the directive included a column number.
> - # Remove "COLUMN:" from the original expression and move it
> + # Remove it from the original expression and move it
> # to the proper place in the search expression.
> - regsub "^\[0-9\]+:" $expmsg "" expmsg
> - set expmsg "$column: $msgprefix\[^\n\]*$expmsg"
> + set expmsg [string range $expmsg [string length $column] end]
> } elseif [string match "" [lindex $newentry 0]] {
> # The specified line number is 0; don't expect a column number.
> - set expmsg "$msgprefix\[^\n\]*$expmsg"
> } else {
> # There is no column number in the search expression, but we
> # should expect one in the message itself.
> - set expmsg "\[0-9\]+: $msgprefix\[^\n\]*$expmsg"
> + set column {[0-9]+:}
> }
> -
> + set expmsg "$column $msgprefix\[^\n\]*$expmsg"
> set newentry [lreplace $newentry 2 2 $expmsg]
> +
> set dg-messages [lreplace ${dg-messages} end end $newentry]
> verbose "process-message:\n${dg-messages}" 2
> }
My Tcl skills aren't great, so hopefully someone else can review this;
CCing Rainer and Mike.
Also, is the proposed syntax for "no columns" OK? (note the tristate
idea above)
Hope this is constructive
Dave
