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