Re: [C++ Patch] More location fixes to grokdeclarator

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2018-06-28 at 02:28 +0200, Paolo Carlini wrote:
> Hi,
> 
> On 28/06/2018 01:31, Jason Merrill wrote:
> > 
> > > +/* Returns the smallest location.  */
> > 
> > This should probably say "...that is not UNKNOWN_LOCATION."
> 
> I agree.
> > Actually, the places you use min_location would seem to work fine
> > with
> > max_location as well.  What are your criteria for choosing one or
> > the
> > other?
> 
> I should have explained that in better detail. I see two different 
> circumstances: either we have error messages where we say something
> like 
> "cannot be both":
> 
> -	  error ("member %qD cannot be declared both %<virtual%> "
> -		 "and %<static%>", dname);
> +	  error_at (max_location (declspecs->locations[ds_virtual],
> +				  declspecs-
> >locations[ds_storage_class]),
> +		    "member %qD cannot be declared both %<virtual%>
> "
> +		    "and %<static%>", dname);
> 
> where, in my opinion, we want to point to the max_location, we want
> to point to where the contradiction shows up in the code. Or, we have
> errors like:
> 
> -	  error ("storage class specified for template parameter
> %qs", name);
> +	  error_at (min_location (declspecs->locations[ds_thread],
> +				  declspecs-
> >locations[ds_storage_class]),
> +		    "storage class specified for template parameter
> %qs",
> +		    name);
> 
> where ill-formed code has either one or two such specifiers (that is
> __thread and/or static) but even one would wrong, thus we want to
> point to the first one, thus min_location (this is in fact a variant
> of the reasoning behind smallest_type_quals_location).
> 
> Did I explain myself clearly enough? If we are going for something
> simple, I would suggest uniformly using min_location, not
> max_location.

If I'm following you right, the idea is that gcc should complain
because two different things in the user's source code contradict each
other.

In such circumstances, I think we ought to try to print *both*
locations, so that we're showing, rather than just telling.

So the user experience might be something like:

test.cc:42:1: error: member 'bar::foo' cannot be declared both 'virtual' and 'static'
 static virtual foo ();
 ^~~~~~ ~~~~~~~

It's possible to send two locations through one diagnostic using
rich_location, via e.g.:

   gcc_rich_location richloc (primary_location);
   richloc.add_range (secondary_location, false);
   error_at (&richloc, "member %qD etc etc"", .../* etc*/);

For this case, I don't think it matters which one is primary and which
is secondary (it merely determines the file:line:column in the
preliminary message of the diagnostic).

Another approach would be to use two diagnostics, for something like:

test.cc:42:8: error: member 'foo' cannot be declared both 'virtual'...
 static virtual foo ();
        ^~~~~~~
test.cc:42:1: note: ...and 'static'
 static virtual foo ();
 ^~~~~~
 
but given that in this case the two things are likely to be close
together in the user's source, I prefer the "two locations in one
diagnostic" approach.

Hope this is constructive
Dave

> By the way, I think I found examples of locations following both the
> patterns above in clang and icc too.
> 
> Thanks,
> Paolo.
>