On Fri, 2015-09-25 at 22:39 +0200, Manuel López-Ibáñez wrote:
> On 25 September 2015 at 22:18, Manuel López-Ibáñez
> <lopeziba...@gmail.com> wrote:
> > On 25 September 2015 at 22:11, David Malcolm <dmalc...@redhat.com> wrote:
>
>
> context->last_location = diagnostic_location (diagnostic, 0);
> - expanded_location s0 = diagnostic_expand_location (diagnostic, 0);
> - expanded_location s1 = { };
> - /* Zero-initialized. This is checked later by
> diagnostic_print_caret_line. */
>
> - if (diagnostic_location (diagnostic, 1) > BUILTINS_LOCATION)
> - s1 = diagnostic_expand_location (diagnostic, 1);
> + if (context->frontend_calls_diagnostic_print_caret_line_p)
> + {
> + /* The GCC < 6 routine. */
> + expanded_location s0 = diagnostic_expand_location (diagnostic, 0);
> + expanded_location s1 = { };
> + /* Zero-initialized. This is checked later by
> + diagnostic_print_caret_line. */
> +
> + if (diagnostic_num_locations (diagnostic) >= 2)
> + s1 = diagnostic->message.m_richloc->get_range (1)->m_start;
>
> - diagnostic_print_caret_line (context, s0, s1,
> - context->caret_chars[0],
> - context->caret_chars[1]);
> + diagnostic_print_caret_line (context, s0, s1,
> + context->caret_chars[0],
> + context->caret_chars[1]);
> + }
> + else
> + /* The GCC >= 6 routine. */
> + diagnostic_print_ranges (context, diagnostic);
> }
>
>
> I haven't had time to look at the patch in detail, so please excuse me
> if this is answered elsewhere.
(nods; the discussion has gotten large).
> Why do you need this hack? The whole point of moving Fortran to the
> common machinery is to not have this duplication.
I attempted to address this in:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01700.html
where I said:
* The Fortran frontend has its own logic for printing multiple
locations, repeatedly calling in to diagnostic_print_caret_line.
I hope the new printing logic is suitable for use by Fortran, but I
wanted to keep the job of "introducing range-capable printing logic"
separate from that of "updating Fortran diagnostics to use it",
since I'm not very familiar with Fortran, and what is desirable
there. Hence to faithfully preserve the existing behavior, I
introduced a flag into the diagnostic_context:
"frontend_calls_diagnostic_print_caret_line_p"
which is set by the Fortran frontend, and makes diagnostic_show_locus
use the existing printing logic. Hopefully that's acceptable,
say, as a migration path.
My recollection is that I saw that the Fortran frontend has logic for
calling into diagnostic_print_caret_line, noticed that the fortran
testsuite has dg- assertions about finding specific messages, and I got
worried that they embed assumptions about how the old printer worked.
Hence I wanted to avoid touching that for the first version, and so in
this patch it's a hybrid of the old Fortran printing code with the new
representation for multiple locations.
Maybe that's a cop-out. Would you prefer that the patch goes all the
way, and that I attempt to eliminate all calls to
diagnostic_print_caret_line from the Fortran FE, and eliminate the old
implementation? (either now, or as a followup patch?) I may need
assistance with that; I suspect that some of the dg- assertions in the
Fortran test suite may need updating.
> Can't the new code print one caret without ranges ever? Something like:
>
> error: expected ';'
> }
> ^
It can handle that just fine. See the examples in line-map.h in the
patch for the kinds of things that a rich_location can represent.
> If it can, then the function responsible for doing that can be called
> by Fortran and it should replace diagnostic_print_caret_line.
>
> Or is it that the new diagnostic_print_ranges cannot print multiple
> carets in the same line? Like this
>
> error: error at (1) and (2)
> adfadfafd asdfdaffa
> 1 2
It can do that too; again see the big comment in line-map.h
> If this is the case, this is a missing functionality that
> diagnostic_print_caret_line already has and that was ready to be used
> in C/C++.
We're good, I believe.
> See example at (O) here:
> https://gcc.gnu.org/wiki/Better_Diagnostics
Pasting it here:
foo.cc: 3:17,3:22: warning: missing braces around initializer for ‘int
[2]’ [-Wmissing-braces]
int a[2][2] = { 0, 1 , 2, 3 }; // { dg-warning "" }
^ ^
{ }
It can support printing carets at both locations. For the braces line,
I'd prefer to do those by explicitly adding a fixit-hints API. I have a
followup patch to do that; see
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00732.html
for an earlier version of said patch, which in fact uses that example in
the unit tests "test_fixit_insert". Although to be fair, I did that
with a single range rather than a pair of carets:
int a[2][2] = { 0, 1 , 2, 3 };
^~~~
{ }
The code supports both approaches (I feel the latter is slightly more
user-friendly as it's more clearly identifying the initializer for
int[2] ...but this is veering off into bike-shed territory).
> In my mind, it should be possible for Fortran to pass to the
> diagnostics machinery two locations with range width 1 (or 0,
> depending how you want to represent a range that covers exactly one
> char) and get a caret line like the example above. Why is this not
> possible?
It is possible.
> Cheers,
>
> Manuel.
Thanks for the comments
Dave
[as noted before, I'm about to disappear on vacation for 10 days, so
replies to followups may be delayed]
