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

On Sep 25, 2015, at 1:11 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> +layout::layout (diagnostic_context * context,
>> +		const diagnostic_info *diagnostic)
>> 
>> [...]
>> 
>> +      if (loc_range->m_finish.file != m_exploc.file)
>> +	continue;
>> +      if (loc_range->m_show_caret_p)
>> +	if (loc_range->m_finish.file != m_exploc.file)
>> +	  continue;
>> 
>> I think the second if clause is redundant.
> 
> Good catch; thanks.

And one other nit.  You don’t validate that the range finishes on or after the start.  Later in the code, you require this to be the case:

bool
layout_range::contains_point (int row, int column) const
{
  gcc_assert (m_start.m_line <= m_finish.m_line);

this code cannot tolerate a range with that property.  So, either, such a range should never be generated, or, if it is to be generated, at least we should declare it awkward.  The below patch declares it to be awkward.  Without this, we ice on completely sane and normal code:

  #define max(i, j) sel((j), (i), (i) < (j))
  yu = max(a2, a3);

giving a valid warning.  In the code, we start on the last line, and finish on the first line.  The underlying problem is that we don’t track macro contexts properly.  sel is a compiler built-in, so, it might be funnier that just a normal function call.  This is from a trunk compiler from 20151201.

So, I was wondering if the problem has been fixed, or, if we should put the below in now, or, would you prefer to try and do up the changes to better track macro expansions?

On Tue, 2015-12-29 at 12:53 -0800, Mike Stump wrote:
> On Sep 25, 2015, at 1:11 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > +layout::layout (diagnostic_context * context,
> >> +		const diagnostic_info *diagnostic)
> >> 
> >> [...]
> >> 
> >> +      if (loc_range->m_finish.file != m_exploc.file)
> >> +	continue;
> >> +      if (loc_range->m_show_caret_p)
> >> +	if (loc_range->m_finish.file != m_exploc.file)
> >> +	  continue;
> >> 
> >> I think the second if clause is redundant.
> > 
> > Good catch; thanks.
> 
> And one other nit.  You don’t validate that the range finishes on or
> after the start.  Later in the code, you require this to be the case:
> 
> bool
> layout_range::contains_point (int row, int column) const
> {
>   gcc_assert (m_start.m_line <= m_finish.m_line);
> 
> this code cannot tolerate a range with that property.  So, either,
> such a range should never be generated, or, if it is to be generated,
> at least we should declare it awkward.  The below patch declares it to
> be awkward.  Without this, we ice on completely sane and normal code:
> 
>   #define max(i, j) sel((j), (i), (i) < (j))
>   yu = max(a2, a3);
> 
> giving a valid warning.  In the code, we start on the last line, and
> finish on the first line.  The underlying problem is that we don’t
> track macro contexts properly.  sel is a compiler built-in, so, it
> might be funnier that just a normal function call.  This is from a
> trunk compiler from 20151201.
> 
> So, I was wondering if the problem has been fixed, or, if we should
> put the below in now, or, would you prefer to try and do up the
> changes to better track macro expansions?

This is PR 68473.

I committed a workaround for it, similar to your one, as r231919 on
2015-12-22; I've been experimenting with a "deeper" fix for it that
would better respect macro expansions, but that might have to wait until
gcc 7.


> diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c
> index 9e51b95..bea8423 100644
> --- a/gcc/diagnostic-show-locus.c
> +++ b/gcc/diagnostic-show-locus.c
> @@ -455,6 +455,9 @@ layout::layout (diagnostic_context * context,
>        if (loc_range->m_show_caret_p)
>         if (loc_range->m_caret.file != m_exploc.file)
>           continue;
> +      /* A range that finishes before it starts is awkward.  */
> +      if (loc_range->m_start.line > loc_range->m_finish.line)
> +       continue;
>  
>        /* Passed all the tests; add the range to m_layout_ranges so
> that
>          it will be printed.  */
>