diff mbox

Patch: fix location in lvalue error

Message ID m3tyhnxwxj.fsf@fleche.redhat.com
State New
Headers show

Commit Message

Tom Tromey Jan. 5, 2011, 2:11 p.m. UTC 
While playing with a refactoring hack, I noticed that locations in
lvalue errors are sometimes incorrect.

E.g., consider this file:

int q(int x)
{
  (x + 0) = 5;
  (x + 0)
    = 7;
}

One error is reported against the '=', but the other is not:

opsy. gcc --syntax-only q.c
q.c: In function ‘q’:
q.c:3:3: error: lvalue required as left operand of assignment
q.c:5:5: error: lvalue required as left operand of assignment


I think it makes sense to report the error using the '=' token.
That is what this patch does.  The new output is:

opsy. gcc --syntax-only q.c
q.c: In function ‘q’:
q.c:3:11: error: lvalue required as left operand of assignment
q.c:5:5: error: lvalue required as left operand of assignment

I did not look at what the C++ compiler emits here.  The patch does not
make it any worse, though.

Built and regtested on x86-64 (compile farm).
Ok?

Tom

2011-01-05  Tom Tromey  <tromey@redhat.com>

	* c-parser.c (c_parser_omp_atomic): Pass location of assignment
	operator to c_finish_omp_atomic.
	* c-typeck.c (lvalue_or_else): Add 'loc' argument.
	(build_unary_op): Update.
	(build_modify_expr): Update.
	(build_asm_expr): Update.

2011-01-05  Tom Tromey  <tromey@redhat.com>

	* typeck.c (cp_build_addr_expr_1): Update call to lvalue_error.
	(lvalue_or_else): Likewise.

2011-01-05  Tom Tromey  <tromey@redhat.com>

	* c-common.h (lvalue_error): Update.
	* c-common.c (lvalue_error): Add 'loc' argument.  Call error_at,
	not error.

Comments

Joseph Myers Jan. 5, 2011, 2:40 p.m. UTC | #1 
On Wed, 5 Jan 2011, Tom Tromey wrote:

> 2011-01-05  Tom Tromey  <tromey@redhat.com>
> 
> 	* c-parser.c (c_parser_omp_atomic): Pass location of assignment
> 	operator to c_finish_omp_atomic.
> 	* c-typeck.c (lvalue_or_else): Add 'loc' argument.
> 	(build_unary_op): Update.
> 	(build_modify_expr): Update.
> 	(build_asm_expr): Update.
> 
> 2011-01-05  Tom Tromey  <tromey@redhat.com>
> 
> 	* c-common.h (lvalue_error): Update.
> 	* c-common.c (lvalue_error): Add 'loc' argument.  Call error_at,
> 	not error.

OK.  (I think the C++ changes are obvious.)
Paolo Bonzini Jan. 5, 2011, 2:51 p.m. UTC | #2 
On 01/05/2011 03:11 PM, Tom Tromey wrote:
> While playing with a refactoring hack, I noticed that locations in
> lvalue errors are sometimes incorrect.
>
> E.g., consider this file:
>
> int q(int x)
> {
>    (x + 0) = 5;
>    (x + 0)
>      = 7;
> }
>
> One error is reported against the '=', but the other is not:
>
> opsy. gcc --syntax-only q.c
> q.c: In function ‘q’:
> q.c:3:3: error: lvalue required as left operand of assignment
> q.c:5:5: error: lvalue required as left operand of assignment
>
>
> I think it makes sense to report the error using the '=' token.
> That is what this patch does.  The new output is:
>
> opsy. gcc --syntax-only q.c
> q.c: In function ‘q’:
> q.c:3:11: error: lvalue required as left operand of assignment
> q.c:5:5: error: lvalue required as left operand of assignment

Interesting, I would have thought that you changed to

gcc --syntax-only q.c
q.c: In function ‘q’:
q.c:3:3: error: lvalue required as left operand of assignment
q.c:4:3: error: lvalue required as left operand of assignment

Since that's where the lvalue is.

Paolo
Tom Tromey Jan. 5, 2011, 3 p.m. UTC | #3 
>>>>> "Paolo" == Paolo Bonzini <bonzini@gnu.org> writes:

>> int q(int x)
>> {
>> (x + 0) = 5;
>> (x + 0)
>> = 7;
>> }

Paolo> Interesting, I would have thought that you changed to

Paolo> gcc --syntax-only q.c
Paolo> q.c: In function ‘q’:
Paolo> q.c:3:3: error: lvalue required as left operand of assignment
Paolo> q.c:4:3: error: lvalue required as left operand of assignment

Paolo> Since that's where the lvalue is.

This was my first thought as well, but GCC doesn't keep enough
information for this to work well.

E.g., in the example above, the location of the lvalue is actually the
location of the "+" token.  And, the location of the "(" token is
discarded during parsing.

Also, if the lvalue is a decl, it won't have a sane location, because
IIRC we use the same tree for both the declaration and all references.

These would be good things to fix for refactoring plugins.  It seems
doable for the C front end; I don't know about C++.

Tom
diff mbox

Patch

Index: c-parser.c
===================================================================
--- c-parser.c	(revision 168451)
+++ c-parser.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* Parser for C and Objective-C.
    Copyright (C) 1987, 1988, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010, 2011
    Free Software Foundation, Inc.
 
    Parser actions based on the old Bison parser; structure somewhat
@@ -9104,6 +9104,9 @@ 
 	  goto saw_error;
 	}
 
+      /* Arrange to pass the location of the assignment operator to
+	 c_finish_omp_atomic.  */
+      loc = c_parser_peek_token (parser)->location;
       c_parser_consume_token (parser);
       {
 	location_t rhs_loc = c_parser_peek_token (parser)->location;
Index: c-typeck.c
===================================================================
--- c-typeck.c	(revision 168451)
+++ c-typeck.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* Build expressions with type checking for C compiler.
    Copyright (C) 1987, 1988, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -98,7 +98,7 @@ 
 static void set_nonincremental_init_from_string (tree, struct obstack *);
 static tree find_init_member (tree, struct obstack *);
 static void readonly_warning (tree, enum lvalue_use);
-static int lvalue_or_else (const_tree, enum lvalue_use);
+static int lvalue_or_else (location_t, const_tree, enum lvalue_use);
 static void record_maybe_used_decl (tree);
 static int comptypes_internal (const_tree, const_tree, bool *, bool *);
 
@@ -3564,7 +3564,8 @@ 
       /* Complain about anything that is not a true lvalue.  In
 	 Objective-C, skip this check for property_refs.  */
       if (!objc_is_property_ref (arg) 
-	  && !lvalue_or_else (arg, ((code == PREINCREMENT_EXPR
+	  && !lvalue_or_else (location,
+			      arg, ((code == PREINCREMENT_EXPR
 				     || code == POSTINCREMENT_EXPR)
 				    ? lv_increment
 				    : lv_decrement)))
@@ -3747,7 +3748,7 @@ 
       /* Anything not already handled and not a true memory reference
 	 or a non-lvalue array is an error.  */
       else if (typecode != FUNCTION_TYPE && !flag
-	       && !lvalue_or_else (arg, lv_addressof))
+	       && !lvalue_or_else (location, arg, lv_addressof))
 	return error_mark_node;
 
       /* Move address operations inside C_MAYBE_CONST_EXPR to simplify
@@ -3905,15 +3906,16 @@ 
 
 /* Return nonzero if REF is an lvalue valid for this language;
    otherwise, print an error message and return zero.  USE says
-   how the lvalue is being used and so selects the error message.  */
+   how the lvalue is being used and so selects the error message.
+   LOCATION is the location at which any error should be reported.  */
 
 static int
-lvalue_or_else (const_tree ref, enum lvalue_use use)
+lvalue_or_else (location_t loc, const_tree ref, enum lvalue_use use)
 {
   int win = lvalue_p (ref);
 
   if (!win)
-    lvalue_error (use);
+    lvalue_error (loc, use);
 
   return win;
 }
@@ -4801,7 +4803,7 @@ 
     return error_mark_node;
 
   /* For ObjC properties, defer this check.  */
-  if (!objc_is_property_ref (lhs) && !lvalue_or_else (lhs, lv_assign))
+  if (!objc_is_property_ref (lhs) && !lvalue_or_else (location, lhs, lv_assign))
     return error_mark_node;
 
   if (TREE_CODE (rhs) == EXCESS_PRECISION_EXPR)
@@ -4851,7 +4853,7 @@ 
 	return result;
 
       /* Else, do the check that we postponed for Objective-C.  */
-      if (!lvalue_or_else (lhs, lv_assign))
+      if (!lvalue_or_else (location, lhs, lv_assign))
 	return error_mark_node;
     }
 
@@ -8479,7 +8481,7 @@ 
 	 get an error.  Gross, but ...  */
       STRIP_NOPS (output);
 
-      if (!lvalue_or_else (output, lv_asm))
+      if (!lvalue_or_else (loc, output, lv_asm))
 	output = error_mark_node;
 
       if (output != error_mark_node
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 168451)
+++ cp/typeck.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* Build expressions with type checking for C++ compiler.
    Copyright (C) 1987, 1988, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
    Free Software Foundation, Inc.
    Hacked by Michael Tiemann (tiemann@cygnus.com)
 
@@ -4756,7 +4756,7 @@ 
       if (kind == clk_none)
 	{
 	  if (complain & tf_error)
-	    lvalue_error (lv_addressof);
+	    lvalue_error (input_location, lv_addressof);
 	  return error_mark_node;
 	}
       if (strict_lvalue && (kind & (clk_rvalueref|clk_class)))
@@ -8219,7 +8219,7 @@ 
   if (kind == clk_none)
     {
       if (complain & tf_error)
-	lvalue_error (use);
+	lvalue_error (input_location, use);
       return 0;
     }
   else if (kind & (clk_rvalueref|clk_class))
Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 168451)
+++ c-family/c-common.c	(working copy)
@@ -1,6 +1,6 @@ 
 /* Subroutines shared by all languages that are variants of C.
    Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
-   2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -8631,27 +8631,28 @@ 
 }
 
 /* Print an error message for an invalid lvalue.  USE says
-   how the lvalue is being used and so selects the error message.  */
+   how the lvalue is being used and so selects the error message.  LOC
+   is the location for the error.  */
 
 void
-lvalue_error (enum lvalue_use use)
+lvalue_error (location_t loc, enum lvalue_use use)
 {
   switch (use)
     {
     case lv_assign:
-      error ("lvalue required as left operand of assignment");
+      error_at (loc, "lvalue required as left operand of assignment");
       break;
     case lv_increment:
-      error ("lvalue required as increment operand");
+      error_at (loc, "lvalue required as increment operand");
       break;
     case lv_decrement:
-      error ("lvalue required as decrement operand");
+      error_at (loc, "lvalue required as decrement operand");
       break;
     case lv_addressof:
-      error ("lvalue required as unary %<&%> operand");
+      error_at (loc, "lvalue required as unary %<&%> operand");
       break;
     case lv_asm:
-      error ("lvalue required in asm statement");
+      error_at (loc, "lvalue required in asm statement");
       break;
     default:
       gcc_unreachable ();
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h	(revision 168451)
+++ c-family/c-common.h	(working copy)
@@ -1,6 +1,6 @@ 
 /* Definitions for c-common.c.
    Copyright (C) 1987, 1993, 1994, 1995, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010, 2011
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -929,7 +929,7 @@ 
 };
 
 extern void readonly_error (tree, enum lvalue_use);
-extern void lvalue_error (enum lvalue_use);
+extern void lvalue_error (location_t, enum lvalue_use);
 extern void invalid_indirection_error (location_t, tree, ref_operator);
 
 extern int complete_array_type (tree *, tree, bool);