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

[David Malcolm <dmalcolm at redhat dot com>]

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.  */
>