Re: [PATCH] v3 of diagnostic_show_locus and rich_location (was Re: [PATCH 2/5] Reimplement diagnostic_show_locus, introducing rich_location classes (v2))

[Dodji Seketeli <dodji at seketeli dot org>]

[Note to libcpp, C, and Fortran maintainers: we still need your input :-)]

Hello,

David Malcolm <dmalcolm@redhat.com> writes:

[...]

> Here's the revised comment I put in the attached patch:

[...]

> +   The class caches the lookup of the color codes for the above.
> +
> +   The class also has responsibility for tracking which of the above is
> +   active, filtering out unnecessary changes.  This allows layout::print_line
> +   to simply request a colorization code for *every* character it prints
> +   thorough this class, and have the filtering be done for it here.

You probably meant "*through* this class" ?

> */

> Hopefully that comment explains the possible states the colorizer can
> have.

Yes it does, great comment, thank you.


> FWIW I have a follow-up patch to add support for fix-it hints, so they
> might be another kind of colorization state.
> (see https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00732.html for the
> earlier version of said patch, in v1 of the kit).

Yeah, I'll comment on that one separatly.

>> Also, I am thinking that there should maybe be a layout::state type,
>> which would have two notional properties (for now): range_index and
>> draw_caret_p. So that this function:
>> 
>> +bool
>> +layout::get_state_at_point (/* Inputs.  */
>> +			    int row, int column,
>> +			    int first_non_ws, int last_non_ws,
>> +			    /* Outputs.  */
>> +			    int *out_range_idx,
>> +			    bool *out_draw_caret_p)
>> 
>> Would take just one output parameter, e.g, a reference to
>> layout::state.
>
> Fixed, though I called it "struct point_state", given that it's coming
> from get_state_at_point.  I passed it by pointer, since AFAIK our coding
> standards don't yet approve of the use of references in the codebase
> (outside of places where we need them e.g. container classes).

Great.  Thanks.

>
> I also added a unit test for a rich_location with two caret locations
> (mimicking one of the Fortran examples), to give us coverage for this
> case:
>
> +void test_multiple_carets (void)
> +{
> +#if 0
> +   x = x + y /* { dg-warning "8: test" } */
> +/* { dg-begin-multiline-output "" }
> +    x = x + y
> +        A   B
> +   { dg-end-multiline-output "" } */
> +#endif
> +}
>
> where the "A" and "B" as caret chars are coming from new code in the
> show_locus unittest plugin.

Yeah, saw that.  Excellent, thanks.

[...]

>> +  if (0)
>> +    show_ruler (context, line_width, m_x_offset);
>> 
>> This should probably be removed from the final code to be committed.
>
> FWIW, the ruler is very helpful to me when debugging the locus-printing
> (e.g. when adding fix-it-hints), and if we remove that if (0) call, we
> get:
>
> warning: âvoid show_ruler(diagnostic_context*, int, int)â defined but
> not used [-Wunused-function]
>
> which will break bootstrap, so perhaps it instead should be an option?
> "-fdiagnostics-show-ruler" or somesuch?
>
> I don't know that it would be helpful to end-users though.
>
> I'd prefer to just keep it in the code with the
>   if (0)
> as-is, since it's useful "scaffolding" for hacking on the code.
>

OK, I understand; though, as Manuel noted elsewhere, you might rename
that function debug_show_ruler and declare it as:

    DEBUG_FUNCTION static void
    debug_show_ruler (diagnostic_context *context, int max_width, int x_offset)
    {
      /* ...  */
    }
to comply with that is generally done in the compiler.

[...]

>> +/* Get the column beyond the rightmost one that could contain a caret or
>> +   range marker, given that we stop rendering at trailing whitespace.  */
>> +
>> +int
>> +layout::get_x_bound_for_row (int row, int caret_column,
>> +			     int last_non_ws)
>> 
>> Please describe what the parameters mean here, especially last_non_ws.
>> I had to read its code to know that last_non_ws was the *column* of
>> the last non white space character.
>
> I renamed it to "last_non_ws_column", and fleshed out the comment

OK.

[...]

>>  void
>>  diagnostic_show_locus (diagnostic_context * context,
>>  		       const diagnostic_info *diagnostic)
>> @@ -75,16 +710,25 @@ diagnostic_show_locus (diagnostic_context * context,
>>      return;
>> 
>> +      /* The GCC 5 routine. */
>> 
>> I'd say the GCC <= 5 routine ;-)
>
>> +  else
>> +    /* The GCC 6 routine.  */
>> 
>> And here, the GCC > 5 routine.
>
> Changed to "GCC < 6" and "GCC >= 6", on the pedantic grounds that e.g.
> 5.1 > 5

OK.

>
>> I would be surprised to see this patch in particular incur any
>> noticeable increase in time and space consumption, but, have you noticed
>> anythying related to that during bootstrap?
>
> I hadn't noticed it, but I wasn't timing.  I'll have a look.

Ok, thanks.

> One possible nit here is that the patch expands locations when
> constructing rich_location instances, and it does that for warnings
> before the logic to ignore them.  So there may be some extra calls there
> that aren't present in trunk, for discarded warnings.  I don't expect
> that to affect the speed of the compiler though (I expect it to be lost
> in the noise).

Fair enough.

> OK for trunk if it passes bootstrap/regrtest?

The core diagnostics bits are IMHO in good shape.  I'd like to see the
discussion with Manuel be resolved before it goes in, though.  And we
still need the "Go" from the FE maintainers.

So I am continuing the discussion with Manuel below.

Manuel LÃpez-IbÃÃez <lopezibanez@gmail.com> writes:

> On 25 September 2015 at 23:15, David Malcolm <dmalcolm@redhat.com> wrote:
>> 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.
>
> It is quite simple, one you understand the logic. Fortran has three
> types of output:
>
> (a) #     [name]:[locus]:
>     #
>     #        some code
>     #              1
>     #     Error: Some error at (1)
>
> which can call the same function used by other FEs to print the caret
> line (I call the caret line, the line that contains the caret
> character/ranges, 1 in this case).
>
> (b) #     [name]:[locus]:
>     #
>     #       some code and some more code
>     #              1       2
>     #     Error: Some error at (1) and (2)
>
>
> which according to what you explained should also be possible by
> calling diagnostic_show_locus with the appropriate location info and
>
> (c) #     [name]:[locus]:
>     #
>     #       some code
>     #              1
>     #     [name]:[locus2]:
>     #
>     #       some other code
>     #         2
>     #     Error: Some error at (1) and (2)
>     # or
>
> which was implemented by calling diagnostic_show_locus with just the
> location of 1, then calling diagnostic_print_caret_line with just the
> expanded_location of 2. I could have just called diagnostic_show_locus
> also to print 2 by overriding diagnostic->location[0] =
> diagnostic->location[1] and caret_char[0] = caret_char[1], but that
> seemed a bit hackish and more expensive (but perhaps less confusing?).
>
> If you have a function that you can call with one or more
> location_t/expanded_location  (or something that can be converted from
> a location_t) and pass explicitly the caret_char, then you just need
> to call that function with the right parameters to get the second part
> of (c). Otherwise, you may simply temporarily do caret_char[0] =
> caret_char[1], before calling the same function that prints the
> caret-line for (a).
>
>> 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.
>
> There is only one call! I just think this hack is really not necessary
> (in fact, it seems more complicated than the alternatives outlined
> above).

I agree that David's approach of adding an impedance adaptation layer in
gcc/diagnostic-show-locus.c to keep the Fortran FE unchanged *for now*
complicates diagnostic-show-locus.c.  But then the benefit is to
break-up the tasks at hand in smaller (less coupled) tasks, making the
whole process more manageable.

IOW, I think the Fortran FE can then be updated later in a follow-up
patch, and then the adaptation layer code can be removed from
diagnostic-show-locus.c *if* we all agree that the Fortran FE can be
adapted to use the foundation being put in place in this current patch.

And I think all the functionalities the Fortran FE needs for that
matter, are supported by this patch.  Please correct me if I am wrong.

> And I'm afraid that once it goes in, it will stay there forever.

:-)

Or maybe not :-)

It looks we are in motion (with some good energy) to change these things
at the moment, thanks to David and you, amongst others.  So I would
agree to bet on a positive outcome of this, and to let the patch go in.
I guess if the Fortran FE doesn't get updated afterwards, I'll be the
one on the hook; I'd take the bet nevertheless.

> You are in a far better position than the Fortran devs to understand
> how to call your new interfaces to get the output you desire.

That is correct.  And hopefully, letting the current patch go in won't
prevent the further needed changes of the FEs to happen, including the
Fortran one.

[...]

Manuel LÃpez-IbÃÃez <lopezibanez@gmail.com> writes:

> On 25 September 2015 at 23:24, David Malcolm <dmalcolm@redhat.com> wrote:
>> On Fri, 2015-09-25 at 23:13 +0200, Manuel LÃpez-IbÃÃez wrote:
>>> +   If SHOW_CARET_P is true, then the range should be rendered with
>>> +   a caret at its starting location.  This
>>> +   is for use by the Fortran frontend, for implementing the
>>> +   "%C" and "%L" format codes.  */
>>> +
>>> +void
>>> +rich_location::set_range (unsigned int idx, source_range src_range,
>>> +              bool show_caret_p, bool overwrite_loc_p)
>>>
>>> I do not understand when is this show_caret_p used by Fortran given
>>> the diagnostic_show_locus code mentioned earlier.
>
> [...]
>> rich_location::set_range exists to ensure that the %C and %L codes used
>> by Fortran (and "+" in the C family of FEs) can write back into the
>> rich_location instance, faithfully emulating the old code that wrote
>> back to
>> struct text_info's:
>>   location_t locations[MAX_LOCATIONS_PER_MESSAGE];
>
> Why Fortran cannot use text->set_location like the other FEs? This way
> you do not need set_range at all.

Yes, it does look to me that this change:

    @@ -938,10 +939,12 @@ gfc_format_decoder (pretty_printer *pp,
            /* If location[0] != UNKNOWN_LOCATION means that we already
               processed one of %C/%L.  */
            int loc_num = text->get_location (0) == UNKNOWN_LOCATION ? 0 : 1;
    -	text->set_location (loc_num,
    -			    linemap_position_for_loc_and_offset (line_table,
    -								 loc->lb->location,
    -								 offset));
    +	source_range range
    +	  = source_range::from_location (
    +	      linemap_position_for_loc_and_offset (line_table,
    +						   loc->lb->location,
    +						   offset));
    +	text->set_range (loc_num, range, true);

is unnecessary because text_info::set_location() got updated as:

    @@ -40,21 +35,17 @@ struct text_info
       va_list *args_ptr;
       int err_no;  /* for %m */
       void **x_data;
    +  rich_location *m_richloc;

    -  inline void set_location (unsigned int index_of_location, location_t loc)
    +  inline void set_location (unsigned int idx, location_t loc, bool caret_p)
       {
    -    gcc_checking_assert (index_of_location < MAX_LOCATIONS_PER_MESSAGE);
    -    this->locations[index_of_location] = loc;
    +    source_range src_range;
    +    src_range.m_start = loc;
    +    src_range.m_finish = loc;
    +    set_range (idx, src_range, caret_p);
       }

So what is needed AIUI is to just add a 'caret_p' argument to the
initial call to:

        text->set_location (loc_num,
    			    linemap_position_for_loc_and_offset (line_table,
    								 loc->lb->location,
    								 offset));

> The other issue that confuses me is that show_caret_p is always true
> when reaching this function via the pretty-printer. Thus, show_caret_p
> is also used by C/C++. In fact, I'm not sure when it can be false.

I think the reason why show_caret_p is always true here is that the
patch updates the existing FE code so that it keeps emitting the *same*
diagnostics as before.  And today, FEs emits diagnostics with ranges
that always starts with a caret.

But I think it's easy to imagine hypothetic (and reasonably probable)
examples where we'd have only one range with a caret, associated to other
ranges with no caret.  The ranges with no carets would thus require the
show_caret_p argument to be non-null.  In fact, patch 2/5 has one of
these examples in a comment in line-map.h:

    +   Example C
    +   *********
    +      a = (foo && bar)
    +           ~~~ ^~ ~~~
    +   This rich location has three ranges:
    +   - Range 0 has its caret and start location at the first "&" and
    +     end at the second "&.
    +   - Range 1 has its start and finish at the "f" and "o" of "foo";
    +     the caret is not flagged for display, but is perhaps at the "f"
    +     of "foo".
    +   - Similarly, range 2 has its start and finish at the "b" and "r" of
    +     "bar"; the caret is not flagged for display, but is perhaps at the
    +     "b" of "bar".

We'll then start seeing cases with show_caret_p being false when *new*
diagnostics are added to FEs to leverage on this new feature.

So I don't think this "show_caret_p" argument's value is an issue, quite
the contrary; it's an interesting feature.

Cheers,

-- 
		Dodji