Adjust locations for RECORD_TYPEs

Adjust locations for RECORD_TYPEs

[Taras Glek]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

Hi,
Currently locations for structs point at {, which is frustrating in 
error messages and painful if one uses GCC to index their codebase (ie 
http://australia.proximity.on.ca/dxr/)

This patch attempts to correct that such that the location of a 
RECORD_TYPE will be the location of the class name.


Taras

[-- Attachment #2: classloc.diff --]
[-- Type: text/plain, Size: 1408 bytes --]

2009-11-25  Taras Glek  <taras@mozilla.com>

	* parser.c (cp_parser_class_specifier): Set class location to that
	of IDENTIFIER_NODE instead of '{'.
	* semantics.c (begin_class_definition): Do not overide locations with less precise ones.


Index: gcc/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c	(revision 154303)
+++ gcc/cp/semantics.c	(working copy)
@@ -2342,9 +2342,6 @@
       pushtag (make_anon_name (), t, /*tag_scope=*/ts_current);
     }
 
-  /* Update the location of the decl.  */
-  DECL_SOURCE_LOCATION (TYPE_NAME (t)) = input_location;
-
   if (TYPE_BEING_DEFINED (t))
     {
       t = make_class_type (TREE_CODE (t));
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 154303)
+++ gcc/cp/parser.c	(working copy)
@@ -15794,6 +15794,7 @@
   tree old_scope = NULL_TREE;
   tree scope = NULL_TREE;
   tree bases;
+  location_t loc;
 
   push_deferring_access_checks (dk_no_deferred);
 
@@ -15811,6 +15812,7 @@
       return error_mark_node;
     }
 
+  loc = input_location;
   /* Look for the `{'.  */
   if (!cp_parser_require (parser, CPP_OPEN_BRACE, "%<{%>"))
     {
@@ -15957,6 +15959,7 @@
   parser->in_unbraced_linkage_specification_p
     = saved_in_unbraced_linkage_specification_p;
 
+  DECL_SOURCE_LOCATION (TYPE_NAME (type)) = loc;
   return type;
 }
 

Re: Adjust locations for RECORD_TYPEs

[Richard Guenther]

On Wed, Nov 25, 2009 at 11:57 PM, Taras Glek <tglek@mozilla.com> wrote:
> Hi,
> Currently locations for structs point at {, which is frustrating in error
> messages and painful if one uses GCC to index their codebase (ie
> http://australia.proximity.on.ca/dxr/)
>
> This patch attempts to correct that such that the location of a RECORD_TYPE
> will be the location of the class name.

What will be the location for anonymous RECORD_TYPEs?

Richard.

Re: Adjust locations for RECORD_TYPEs

[Taras Glek]

On 11/25/2009 03:00 PM, Richard Guenther wrote:
> On Wed, Nov 25, 2009 at 11:57 PM, Taras Glek<tglek@mozilla.com>  wrote:
>    
>> Hi,
>> Currently locations for structs point at {, which is frustrating in error
>> messages and painful if one uses GCC to index their codebase (ie
>> http://australia.proximity.on.ca/dxr/)
>>
>> This patch attempts to correct that such that the location of a RECORD_TYPE
>> will be the location of the class name.
>>      
> What will be the location for anonymous RECORD_TYPEs?
>    
End of struct/class token.

Taras

Re: Adjust locations for RECORD_TYPEs

[Gabriel Dos Reis]

On Wed, Nov 25, 2009 at 5:02 PM, Taras Glek <tglek@mozilla.com> wrote:
> On 11/25/2009 03:00 PM, Richard Guenther wrote:
>>
>> On Wed, Nov 25, 2009 at 11:57 PM, Taras Glek<tglek@mozilla.com>  wrote:
>>
>>>
>>> Hi,
>>> Currently locations for structs point at {, which is frustrating in error
>>> messages and painful if one uses GCC to index their codebase (ie
>>> http://australia.proximity.on.ca/dxr/)
>>>
>>> This patch attempts to correct that such that the location of a
>>> RECORD_TYPE
>>> will be the location of the class name.
>>>
>>
>> What will be the location for anonymous RECORD_TYPEs?
>>
>
> End of struct/class token.

Well, that looks weird to me, compared to the existing behaviour.

-- Gaby

Re: Adjust locations for RECORD_TYPEs

[Taras Glek]

On 11/25/2009 03:36 PM, Gabriel Dos Reis wrote:
> On Wed, Nov 25, 2009 at 5:02 PM, Taras Glek<tglek@mozilla.com>  wrote:
>    
>> On 11/25/2009 03:00 PM, Richard Guenther wrote:
>>      
>>> On Wed, Nov 25, 2009 at 11:57 PM, Taras Glek<tglek@mozilla.com>    wrote:
>>>
>>>        
>>>> Hi,
>>>> Currently locations for structs point at {, which is frustrating in error
>>>> messages and painful if one uses GCC to index their codebase (ie
>>>> http://australia.proximity.on.ca/dxr/)
>>>>
>>>> This patch attempts to correct that such that the location of a
>>>> RECORD_TYPE
>>>> will be the location of the class name.
>>>>
>>>>          
>>> What will be the location for anonymous RECORD_TYPEs?
>>>
>>>        
>> End of struct/class token.
>>      
> Well, that looks weird to me, compared to the existing behaviour.
>
>    
Existing behavior is problematic for
class foo
: public parent
{
}
programming style because any error messages/etc point to {, which often 
means that { is on top of the screen when you jump to that location, 
making one scroll up to get some context.

I don't see how this is better than going to either the class name or 
class tag in anonymous case.

Alternatively we could make the location reflect the beginning of the 
'class' token.

Taras

Re: Adjust locations for RECORD_TYPEs

[Gabriel Dos Reis]

On Wed, Nov 25, 2009 at 5:41 PM, Taras Glek <tglek@mozilla.com> wrote:
> On 11/25/2009 03:36 PM, Gabriel Dos Reis wrote:
>>
>> On Wed, Nov 25, 2009 at 5:02 PM, Taras Glek<tglek@mozilla.com>  wrote:
>>
>>>
>>> On 11/25/2009 03:00 PM, Richard Guenther wrote:
>>>
>>>>
>>>> On Wed, Nov 25, 2009 at 11:57 PM, Taras Glek<tglek@mozilla.com>
>>>>  wrote:
>>>>
>>>>
>>>>>
>>>>> Hi,
>>>>> Currently locations for structs point at {, which is frustrating in
>>>>> error
>>>>> messages and painful if one uses GCC to index their codebase (ie
>>>>> http://australia.proximity.on.ca/dxr/)
>>>>>
>>>>> This patch attempts to correct that such that the location of a
>>>>> RECORD_TYPE
>>>>> will be the location of the class name.
>>>>>
>>>>>
>>>>
>>>> What will be the location for anonymous RECORD_TYPEs?
>>>>
>>>>
>>>
>>> End of struct/class token.
>>>
>>
>> Well, that looks weird to me, compared to the existing behaviour.
>>
>>
>
> Existing behavior is problematic for
> class foo
> : public parent
> {
> }
> programming style because any error messages/etc point to {, which often
> means that { is on top of the screen when you jump to that location, making
> one scroll up to get some context.

'{' is where the type definition is, and it is where it works well for
both named and unnamed user-defined type.

If your concern is for supporting a some coding standard, I think
that is should be the job of external tools, and all GCC has to do
is to emit enough location information -- so that anyone can
guide their IDE to support the favorite coding standards.

Consequently, I don't see this change as appropriate.

-- Gaby

Re: Adjust locations for RECORD_TYPEs

[Taras Glek]

On 11/25/2009 04:03 PM, Gabriel Dos Reis wrote:
> On Wed, Nov 25, 2009 at 5:41 PM, Taras Glek<tglek@mozilla.com>  wrote:
>    
>> On 11/25/2009 03:36 PM, Gabriel Dos Reis wrote:
>>      
>>> On Wed, Nov 25, 2009 at 5:02 PM, Taras Glek<tglek@mozilla.com>    wrote:
>>>
>>>        
>>>> On 11/25/2009 03:00 PM, Richard Guenther wrote:
>>>>
>>>>          
>>>>> On Wed, Nov 25, 2009 at 11:57 PM, Taras Glek<tglek@mozilla.com>
>>>>>   wrote:
>>>>>
>>>>>
>>>>>            
>>>>>> Hi,
>>>>>> Currently locations for structs point at {, which is frustrating in
>>>>>> error
>>>>>> messages and painful if one uses GCC to index their codebase (ie
>>>>>> http://australia.proximity.on.ca/dxr/)
>>>>>>
>>>>>> This patch attempts to correct that such that the location of a
>>>>>> RECORD_TYPE
>>>>>> will be the location of the class name.
>>>>>>
>>>>>>
>>>>>>              
>>>>> What will be the location for anonymous RECORD_TYPEs?
>>>>>
>>>>>
>>>>>            
>>>> End of struct/class token.
>>>>
>>>>          
>>> Well, that looks weird to me, compared to the existing behaviour.
>>>
>>>
>>>        
>> Existing behavior is problematic for
>> class foo
>> : public parent
>> {
>> }
>> programming style because any error messages/etc point to {, which often
>> means that { is on top of the screen when you jump to that location, making
>> one scroll up to get some context.
>>      
> '{' is where the type definition is, and it is where it works well for
> both named and unnamed user-defined type.
>    
I disagree, I think it is reasonable consider class name and inheritance 
info part of the definition.
> If your concern is for supporting a some coding standard, I think
> that is should be the job of external tools, and all GCC has to do
> is to emit enough location information -- so that anyone can
> guide their IDE to support the favorite coding standards.
>
> Consequently, I don't see this change as appropriate.
>    
My concern is the location does not show the class name and other 
relevant info. I'm not trying to support any particular coding standard, 
just showing that the current method is insufficient. Unfortunately due 
to the complexity of the language, parsing C++ is a job that external 
tools should leave to GCC, so if GCC does not provide sufficient 
location resolution, there is little that the tools can do.

See
http://australia.proximity.on.ca/dxr/search.cgi?tree=mozilla-central&amp;derived=nsISerializable
and follow the link to nsStandardURL. Note how the top of the class is 
scrolled off the screen. The same clipping of context happens in error 
messages.

I don't see how pointing at where the type definition actually begins is 
any worse than the current behavior.

Taras

Re: Adjust locations for RECORD_TYPEs

[Jason Merrill]

It would be better to use type_start_token->location in 
cp_parser_class_head rather than store input_location.

Jason

Re: Adjust locations for RECORD_TYPEs

[Taras Glek]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On 11/26/2009 06:55 AM, Jason Merrill wrote:
> It would be better to use type_start_token->location in 
> cp_parser_class_head rather than store input_location.

You are right. This changes behavior such that anonymous RECORD_TYPE 
location points at { or class name when that is available.

2009-11-30  Taras Glek <taras@mozilla.com>

     * parser.c (cp_parser_class_specifier): Set class location to that
     of IDENTIFIER_NODE instead of '{'.

2009-11-30  Taras Glek <taras@mozilla.com>

     * semantics.c (begin_class_definition): Do not overide locations 
with less precise ones.



[-- Attachment #2: svn.diff --]
[-- Type: text/plain, Size: 851 bytes --]

Index: gcc/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c	(revision 154303)
+++ gcc/cp/semantics.c	(working copy)
@@ -2342,9 +2342,6 @@
       pushtag (make_anon_name (), t, /*tag_scope=*/ts_current);
     }
 
-  /* Update the location of the decl.  */
-  DECL_SOURCE_LOCATION (TYPE_NAME (t)) = input_location;
-
   if (TYPE_BEING_DEFINED (t))
     {
       t = make_class_type (TREE_CODE (t));
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 154303)
+++ gcc/cp/parser.c	(working copy)
@@ -16366,6 +16366,8 @@
       end_specialization ();
       --parser->num_template_parameter_lists;
     }
+
+  DECL_SOURCE_LOCATION (TYPE_NAME (type)) = type_start_token->location;
   *attributes_p = attributes;
   return type;
 }

Re: Adjust locations for RECORD_TYPEs

[Gabriel Dos Reis]

On Mon, Nov 30, 2009 at 1:11 PM, Taras Glek <tglek@mozilla.com> wrote:
> On 11/26/2009 06:55 AM, Jason Merrill wrote:
>>
>> It would be better to use type_start_token->location in
>> cp_parser_class_head rather than store input_location.
>
> You are right. This changes behavior such that anonymous RECORD_TYPE
> location points at { or class name when that is available.

Ok, thay is better.

-- Gaby

Re: Adjust locations for RECORD_TYPEs

[Jason Merrill]

OK.

Jason

Re: Adjust locations for RECORD_TYPEs

[Richard Guenther]

On Mon, Nov 30, 2009 at 9:24 PM, Jason Merrill <jason@redhat.com> wrote:
> OK.

This seems to have caused a lot of testsuite fails.  Did you properly test the
patch?

http://gcc.gnu.org/ml/gcc-regression/2009-12/msg00018.html

Richard.
> Jason
>

Re: Adjust locations for RECORD_TYPEs

[Taras Glek]

On 12/02/2009 06:04 AM, Richard Guenther wrote:
> On Mon, Nov 30, 2009 at 9:24 PM, Jason Merrill<jason@redhat.com>  wrote:
>    
>> OK.
>>      
> This seems to have caused a lot of testsuite fails.  Did you properly test the
> patch?
>
> http://gcc.gnu.org/ml/gcc-regression/2009-12/msg00018.html
>    
Looks like I did not test the patch properly. I backed it out, sorry for 
the breakage, I'll be more careful next time.

Taras

Re: Adjust locations for RECORD_TYPEs

[Dominique Dhumieres]

This caused pr42254

Dominique