This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, 2018-06-27 at 21:13 -0400, David Malcolm wrote:
> 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.

Or to put in another way, if two things in the user's source contradict
each other, we should show the user both.  The user is going to have to
decide to delete one (or both) of them, and we don't know which one,
but at least by showing both it helps him/her take their next action.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]