On Fri, 2020-01-31 at 14:31 -0500, Lewis Hyatt wrote: > Hello- > > Here is the second patch that I mentioned when I submitted the other > related > patch (which is awaiting review): > https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01626.html.
Sorry about that; I'm v. busy with analyzer bugs right now. > This second patch > is based on top of the first one and it closes out PR49973 and > PR86904 by > adding the new option -fdiagnostics-column-unit=[display|byte]. This > allows > to specify whether columns are output as simple byte counts (the > current > behavior), or as display columns including handling multibyte > characters and > tabs. The patch makes display columns the new default. Additionally, > a > second new option -fdiagnostics-column-origin is added, which allows > to make > the column 0-based (or N-based for any N) instead of 1-based. The > default > remains at 1-based as it is now. > > A number of testcases were explicitly testing for the old behavior, > so I > have updated them to test for the new behavior instead, since the > column > number adjusted for tabs is more natural to test for, and matches > what > editors typically show (give or take 1 for the origin convention). > > One other testcase (go.dg/arrayclear.go) was a bit of an oddity. It > failed > after this patch, although it doesn't test for any column numbers. > The > answer turned out to be, this test checks for identical error text on > two > different lines. When the column units are changed to display > columns, then > the column of the second error happens to match the line of the first > one. dejagnu then misinterprets the second error as if it matched the > location of the first one (it doesn't distinguish whether it checks > for the > line number or the column number in the output). I added a comment to > the > test explaining the situation; since adding the comment has the side > effect > of making the first line number no longer match the second column > number, it > also makes the test pass again. > > It wasn't quite clear to me whether this change was appropriate for > GCC 10 > or not at this point. We discussed it a couple months ago here: > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02171.html. Either way, > I hope > it isn't a problem that I submitted the patch for review now, whether > it > will end up in 10 or 11. Please let me know what's normally expected? > Thanks! Thanks Lewis. This patch looks very promising, but should wait until gcc 11; we're trying to stabilize gcc 10 right now (I'm knee-deep in analyzer bug- fixing, so I don't want to add any more diagnostics changes). > gcc/ChangeLog: > > 2020-01-31 Lewis Hyatt <lhy...@gmail.com> > Please reference the PRs here [...] > gcc/testsuite/ChangeLog: > > 2020-01-31 Lewis Hyatt <lhy...@gmail.com> Likewise here. [...] > diff --git a/gcc/common.opt b/gcc/common.opt > index 630c380bd6a..657985450c2 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -1309,6 +1309,14 @@ Enum(diagnostic_url_rule) String(always) > Value(DIAGNOSTICS_URL_YES) > EnumValue > Enum(diagnostic_url_rule) String(auto) Value(DIAGNOSTICS_URL_AUTO) > > +fdiagnostics-column-unit= > +Common Joined RejectNegative Enum(diagnostics_column_unit) > +-fdiagnostics-column-unit=[display|byte] Select units for column numbers. Should this line mention the default? > +fdiagnostics-column-origin= > +Common Joined RejectNegative UInteger > +-fdiagnostics-column-origin=<number> Set the number of the first column. > Default 1-based. These new options should be documented in gcc/doc/invoke.texi. [...] > @@ -43,21 +44,23 @@ static json::array *cur_children_array; > /* Generate a JSON object for LOC. */ > > json::value * > -json_from_expanded_location (location_t loc) > +json_from_expanded_location (diagnostic_context *context, location_t loc) > { > expanded_location exploc = expand_location (loc); > json::object *result = new json::object (); > if (exploc.file) > result->set ("file", new json::string (exploc.file)); > result->set ("line", new json::integer_number (exploc.line)); > - result->set ("column", new json::integer_number (exploc.column)); > + const int col = diagnostic_converted_column (context, exploc); > + result->set ("column", new json::integer_number (col)); I wonder if the JSON output format should show *both* values: perhaps add fields "byte-column" and "display-column", and retain the field "column", which would follow -fdiagnostics-column-unit? [...] > @@ -219,6 +220,8 @@ diagnostic_initialize (diagnostic_context *context, int > n_opts) > context->min_margin_width = 0; > context->show_ruler_p = false; > context->parseable_fixits_p = false; > + context->column_unit = DIAGNOSTICS_COLUMN_UNIT_DISPLAY; > + context->column_adj = 0; I'm not sure, but I think I prefer it if we store the column origin instead, rather than an offset relative to an origin of 1. [...] > @@ -338,8 +341,37 @@ diagnostic_get_color_for_kind (diagnostic_t kind) > return diagnostic_kind_color[kind]; > } > > +/* Given an expanded_location, convert the column (which is in 1-based bytes) > + to the requested units and origin. Return -1 if the column is > + invalid (<= 0). */ > +int > +diagnostic_converted_column (diagnostic_context *context, expanded_location > s) > +{ > + if (s.column <= 0) > + return -1; > + > + int col; ...so this would be one_based_col. > + switch (context->column_unit) > + { > + case DIAGNOSTICS_COLUMN_UNIT_DISPLAY: > + col = location_compute_display_column (s); > + break; > + > + case DIAGNOSTICS_COLUMN_UNIT_BYTE: > + col = s.column; > + break; > + > + default: > + gcc_unreachable (); > + } > + > + return col + context->column_adj; ...and this would be (I think): return context->column_origin + one_based_col - 1; It would be doing the -1 each time, but maybe it's conceptually clearer? I'm not sure. [...] > @@ -882,8 +930,10 @@ print_parseable_fixits (pretty_printer *pp, > rich_location *richloc) > location_t next_loc = hint->get_next_loc (); > expanded_location next_exploc = expand_location (next_loc); > pp_printf (pp, ":{%i:%i-%i:%i}:", > - start_exploc.line, start_exploc.column, > - next_exploc.line, next_exploc.column); > + start_exploc.line, > + diagnostic_converted_column (context, start_exploc), > + next_exploc.line, > + diagnostic_converted_column (context, next_exploc)); > print_escaped_string (pp, hint->get_string ()); > pp_newline (pp); > } If we're going to change the output of parseable fixits, that takes us away from bug-for-bug-compatibility with clang in this area. That should be documented, at least. [...] There's selftest coverage which is good; it would be good to *also* have a few simple DejaGnu-based tests, showing the explicit use of both units, and trying some offset values, with some lines with tabs, some with spaces (if nothing else to verify that the option-parsing is wired up correctly). I'm nit-picking - apart from the lack of docs, this looks very promising. But as I said earlier, this should wait until gcc 11. Thanks Dave
