diff mbox

[C++,Patch/RFC] PR 54194

Message ID 5073804E.6070905@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Oct. 9, 2012, 1:39 a.m. UTC 
Hi again,

On 10/08/2012 11:44 PM, Jason Merrill wrote:
> On 10/08/2012 05:31 PM, Paolo Carlini wrote:
>> So, there is a serious difficulty, I'm afraid: for the example at issue,
>> EXPR_LOCATION (arg_left) is 0 not any meaningful value. And of course
>> EXPR_LOC_OR_HERE would not be better in this case, would give
>> input_location. So, what do you think? Shall we just go with the loc for
>> now, or you think there is something relatively safe we can try?
> Looks like we still need to SET_EXPR_LOCATION in cp_build_binary_op.  
> It would also be good to have a macro/function like EXPR_LOC_OR_HERE 
> that lets you provide the location to use if the expression doesn't 
> have one.
Thus I'm finishing testing the below (C++ is Ok)

It works pretty well, IMHO: for actual expressions we can have the 
column number of the operand (which is now fixed up in 
cp_parser_binary_expression), otherwise, for eg constants, we fall back 
to the column number of the operator. As a bonus, in templates like 
Wparentheses-26.C we even get the line number right.

Thanks!
Paolo.

////////////////////////////
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/54194
	* tree.h: Add EXPR_LOC_OR_LOC.

c-family/
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/54194
	* c-common.c (warn_about_parentheses): Add location_t parameter;
	use EXPR_LOC_OR_LOC.
	* c-common.h: Update declaration.

c/
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/54194
	* c-typeck.c (parser_build_binary_op): Update warn_about_parentheses
	call.

/cp
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/54194
	* typeck.c (build_x_binary_op): Update warn_about_parentheses call.
	* parser.c (cp_parser_binary_expression): Use SET_EXPR_LOCATION
	on current.lhs.

/testsuite
2012-10-09  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/54194
	* g++.dg/warn/Wparentheses-26.C: Adjust.
	* g++.dg/warn/Wparentheses-27.C: New.

Comments

Jason Merrill Oct. 9, 2012, 2:01 a.m. UTC | #1 
OK.

Jason
diff mbox

Patch

Index: testsuite/g++.dg/warn/Wparentheses-26.C
===================================================================
--- testsuite/g++.dg/warn/Wparentheses-26.C	(revision 192224)
+++ testsuite/g++.dg/warn/Wparentheses-26.C	(working copy)
@@ -4,23 +4,23 @@ 
 template<int i, int j = ((i + 7) >> 3)> class foo1 { };
 typedef foo1<10> bar1;
 
-template<int i, int j = (i + 7 >> 3)> class foo2 { };
-typedef foo2<10> bar2;  // { dg-warning "suggest parentheses around '\\+'" }
+template<int i, int j = (i + 7 >> 3)> class foo2 { };  // { dg-warning "suggest parentheses around '\\+'" }
+typedef foo2<10> bar2;
 
 template<int i, int j = (100 >> (i + 2))> class foo3 { };
 typedef foo3<3> bar3;
 
-template<int i, int j = (100 >> i + 2)> class foo4 { }; 
-typedef foo4<3> bar4;   // { dg-warning "suggest parentheses around '\\+'" }
+template<int i, int j = (100 >> i + 2)> class foo4 { }; // { dg-warning "suggest parentheses around '\\+'" }
+typedef foo4<3> bar4;
 
 template<int i, int j = (i + 7) | 3> class foo5 { };
 typedef foo5<10> bar5;
 
-template<int i, int j = i + 7 | 3> class foo6 { };
-typedef foo6<10> bar6;  // { dg-warning "suggest parentheses around arithmetic" }
+template<int i, int j = i + 7 | 3> class foo6 { }; // { dg-warning "suggest parentheses around arithmetic" }
+typedef foo6<10> bar6;
 
 template<int i, int j = 3 | (i + 7)> class foo7 { };
 typedef foo7<10> bar7;
 
-template<int i, int j = 3 | i + 7> class foo8 { };
-typedef foo8<10> bar8;  // { dg-warning "suggest parentheses around arithmetic" }
+template<int i, int j = 3 | i + 7> class foo8 { }; // { dg-warning "suggest parentheses around arithmetic" } 
+typedef foo8<10> bar8;
Index: testsuite/g++.dg/warn/Wparentheses-27.C
===================================================================
--- testsuite/g++.dg/warn/Wparentheses-27.C	(revision 0)
+++ testsuite/g++.dg/warn/Wparentheses-27.C	(working copy)
@@ -0,0 +1,8 @@ 
+// PR c++/54194
+// { dg-options "-Wparentheses" }
+
+int main()
+{
+  char in[4] = { 0 };
+  in[1] = in[1] & 0x0F | ((in[3] & 0x3C) << 2); // { dg-warning "17:suggest parentheses" }
+}
Index: c/c-typeck.c
===================================================================
--- c/c-typeck.c	(revision 192224)
+++ c/c-typeck.c	(working copy)
@@ -3261,7 +3261,8 @@  parser_build_binary_op (location_t location, enum
   /* Check for cases such as x+y<<z which users are likely
      to misinterpret.  */
   if (warn_parentheses)
-    warn_about_parentheses (code, code1, arg1.value, code2, arg2.value);
+    warn_about_parentheses (input_location, code,
+			    code1, arg1.value, code2, arg2.value);
 
   if (warn_logical_op)
     warn_logical_operator (input_location, code, TREE_TYPE (result.value),
Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 192224)
+++ c-family/c-common.c	(working copy)
@@ -10527,7 +10527,7 @@  warn_array_subscript_with_type_char (tree index)
    was enclosed in parentheses.  */
 
 void
-warn_about_parentheses (enum tree_code code,
+warn_about_parentheses (location_t loc, enum tree_code code,
 			enum tree_code code_left, tree arg_left,
 			enum tree_code code_right, tree arg_right)
 {
@@ -10547,104 +10547,147 @@  void
   switch (code)
     {
     case LSHIFT_EXPR:
-      if (code_left == PLUS_EXPR || code_right == PLUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<+%> inside %<<<%>");
-      else if (code_left == MINUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<-%> inside %<<<%>");
+      if (code_left == PLUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<+%> inside %<<<%>");
+      else if (code_right == PLUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<+%> inside %<<<%>");
+      else if (code_left == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<-%> inside %<<<%>");
+      else if (code_right == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<-%> inside %<<<%>");
       return;
 
     case RSHIFT_EXPR:
-      if (code_left == PLUS_EXPR || code_right == PLUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<+%> inside %<>>%>");
-      else if (code_left == MINUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<-%> inside %<>>%>");
+      if (code_left == PLUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<+%> inside %<>>%>");
+      else if (code_right == PLUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<+%> inside %<>>%>");
+      else if (code_left == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<-%> inside %<>>%>");
+      else if (code_right == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<-%> inside %<>>%>");
       return;
 
     case TRUTH_ORIF_EXPR:
-      if (code_left == TRUTH_ANDIF_EXPR || code_right == TRUTH_ANDIF_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<&&%> within %<||%>");
+      if (code_left == TRUTH_ANDIF_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<&&%> within %<||%>");
+      else if (code_right == TRUTH_ANDIF_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		    "suggest parentheses around %<&&%> within %<||%>");
       return;
 
     case BIT_IOR_EXPR:
       if (code_left == BIT_AND_EXPR || code_left == BIT_XOR_EXPR
-	  || code_left == PLUS_EXPR || code_left == MINUS_EXPR
-	  || code_right == BIT_AND_EXPR || code_right == BIT_XOR_EXPR
-	  || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
+	  || code_left == PLUS_EXPR || code_left == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around arithmetic in operand of %<|%>");
+      else if (code_right == BIT_AND_EXPR || code_right == BIT_XOR_EXPR
+	       || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around arithmetic in operand of %<|%>");
       /* Check cases like x|y==z */
-      else if (TREE_CODE_CLASS (code_left) == tcc_comparison
-	       || TREE_CODE_CLASS (code_right) == tcc_comparison)
-	warning (OPT_Wparentheses,
+      else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around comparison in operand of %<|%>");
+      else if (TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around comparison in operand of %<|%>");
       /* Check cases like !x | y */
       else if (code_left == TRUTH_NOT_EXPR
 	       && !APPEARS_TO_BE_BOOLEAN_EXPR_P (code_right, arg_right))
-	warning (OPT_Wparentheses, "suggest parentheses around operand of "
-		 "%<!%> or change %<|%> to %<||%> or %<!%> to %<~%>");
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around operand of "
+		    "%<!%> or change %<|%> to %<||%> or %<!%> to %<~%>");
       return;
 
     case BIT_XOR_EXPR:
       if (code_left == BIT_AND_EXPR
-	  || code_left == PLUS_EXPR || code_left == MINUS_EXPR
-	  || code_right == BIT_AND_EXPR
-	  || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
+	  || code_left == PLUS_EXPR || code_left == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around arithmetic in operand of %<^%>");
+      else if (code_right == BIT_AND_EXPR
+	       || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around arithmetic in operand of %<^%>");
       /* Check cases like x^y==z */
-      else if (TREE_CODE_CLASS (code_left) == tcc_comparison
-	       || TREE_CODE_CLASS (code_right) == tcc_comparison)
-	warning (OPT_Wparentheses,
+      else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around comparison in operand of %<^%>");
+      else if (TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around comparison in operand of %<^%>");
       return;
 
     case BIT_AND_EXPR:
-      if (code_left == PLUS_EXPR || code_right == PLUS_EXPR)
-	warning (OPT_Wparentheses,
+      if (code_left == PLUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around %<+%> in operand of %<&%>");
-      else if (code_left == MINUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
+      else if (code_right == PLUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around %<+%> in operand of %<&%>");
+      else if (code_left == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around %<-%> in operand of %<&%>");
+      else if (code_right == MINUS_EXPR)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around %<-%> in operand of %<&%>");
       /* Check cases like x&y==z */
-      else if (TREE_CODE_CLASS (code_left) == tcc_comparison
-	       || TREE_CODE_CLASS (code_right) == tcc_comparison)
-	warning (OPT_Wparentheses,
+      else if (TREE_CODE_CLASS (code_left) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around comparison in operand of %<&%>");
+      else if (TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around comparison in operand of %<&%>");
       /* Check cases like !x & y */
       else if (code_left == TRUTH_NOT_EXPR
 	       && !APPEARS_TO_BE_BOOLEAN_EXPR_P (code_right, arg_right))
-	warning (OPT_Wparentheses, "suggest parentheses around operand of "
-		 "%<!%> or change %<&%> to %<&&%> or %<!%> to %<~%>");
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+		    "suggest parentheses around operand of "
+		    "%<!%> or change %<&%> to %<&&%> or %<!%> to %<~%>");
       return;
 
     case EQ_EXPR:
-      if (TREE_CODE_CLASS (code_left) == tcc_comparison
-          || TREE_CODE_CLASS (code_right) == tcc_comparison)
-	warning (OPT_Wparentheses,
+      if (TREE_CODE_CLASS (code_left) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around comparison in operand of %<==%>");
+      else if (TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around comparison in operand of %<==%>");
       return;
     case NE_EXPR:
-      if (TREE_CODE_CLASS (code_left) == tcc_comparison
-          || TREE_CODE_CLASS (code_right) == tcc_comparison)
-	warning (OPT_Wparentheses,
+      if (TREE_CODE_CLASS (code_left) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
 		 "suggest parentheses around comparison in operand of %<!=%>");
+      else if (TREE_CODE_CLASS (code_right) == tcc_comparison)
+	warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+		 "suggest parentheses around comparison in operand of %<!=%>");
       return;
 
     default:
-      if (TREE_CODE_CLASS (code) == tcc_comparison
-	   && ((TREE_CODE_CLASS (code_left) == tcc_comparison
+      if (TREE_CODE_CLASS (code) == tcc_comparison)
+	{
+	  if (TREE_CODE_CLASS (code_left) == tcc_comparison
 		&& code_left != NE_EXPR && code_left != EQ_EXPR
 		&& INTEGRAL_TYPE_P (TREE_TYPE (arg_left)))
-	       || (TREE_CODE_CLASS (code_right) == tcc_comparison
+	    warning_at (EXPR_LOC_OR_LOC (arg_left, loc), OPT_Wparentheses,
+			"comparisons like %<X<=Y<=Z%> do not "
+			"have their mathematical meaning");
+	  else if (TREE_CODE_CLASS (code_right) == tcc_comparison
 		   && code_right != NE_EXPR && code_right != EQ_EXPR
-		   && INTEGRAL_TYPE_P (TREE_TYPE (arg_right)))))
-	warning (OPT_Wparentheses, "comparisons like %<X<=Y<=Z%> do not "
-		 "have their mathematical meaning");
+		   && INTEGRAL_TYPE_P (TREE_TYPE (arg_right)))
+	    warning_at (EXPR_LOC_OR_LOC (arg_right, loc), OPT_Wparentheses,
+			"comparisons like %<X<=Y<=Z%> do not "
+			"have their mathematical meaning");
+	}
       return;
     }
 #undef NOT_A_BOOLEAN_EXPR_P
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h	(revision 192224)
+++ c-family/c-common.h	(working copy)
@@ -989,7 +989,8 @@  extern tree builtin_type_for_size (int, bool);
 extern void c_common_mark_addressable_vec (tree);
 
 extern void warn_array_subscript_with_type_char (tree);
-extern void warn_about_parentheses (enum tree_code,
+extern void warn_about_parentheses (location_t,
+				    enum tree_code,
 				    enum tree_code, tree,
 				    enum tree_code, tree);
 extern void warn_for_unused_label (tree label);
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 192224)
+++ cp/parser.c	(working copy)
@@ -7472,6 +7472,8 @@  cp_parser_binary_expression (cp_parser* parser, bo
 					 rhs, rhs_type, &overload,
 					 tf_warning_or_error);
       current.lhs_type = current.tree_type;
+      if (EXPR_P (current.lhs))
+	SET_EXPR_LOCATION (current.lhs, current.loc);
 
       /* If the binary operator required the use of an overloaded operator,
 	 then this expression cannot be an integral constant-expression.
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 192224)
+++ cp/typeck.c	(working copy)
@@ -3680,7 +3680,8 @@  build_x_binary_op (location_t loc, enum tree_code
       && !error_operand_p (arg2)
       && (code != LSHIFT_EXPR
 	  || !CLASS_TYPE_P (TREE_TYPE (arg1))))
-    warn_about_parentheses (code, arg1_code, orig_arg1, arg2_code, orig_arg2);
+    warn_about_parentheses (loc, code, arg1_code, orig_arg1,
+			    arg2_code, orig_arg2);
 
   if (processing_template_decl && expr != error_mark_node)
     return build_min_non_dep (code, expr, orig_arg1, orig_arg2);
Index: tree.h
===================================================================
--- tree.h	(revision 192224)
+++ tree.h	(working copy)
@@ -1619,7 +1619,10 @@  struct GTY(()) tree_constructor {
   != UNKNOWN_LOCATION)
 /* The location to be used in a diagnostic about this expression.  Do not
    use this macro if the location will be assigned to other expressions.  */
-#define EXPR_LOC_OR_HERE(NODE) (EXPR_HAS_LOCATION (NODE) ? (NODE)->exp.locus : input_location)
+#define EXPR_LOC_OR_HERE(NODE) (EXPR_HAS_LOCATION (NODE) \
+				? (NODE)->exp.locus : input_location)
+#define EXPR_LOC_OR_LOC(NODE, LOCUS) (EXPR_HAS_LOCATION (NODE) \
+				      ? (NODE)->exp.locus : (LOCUS))
 #define EXPR_FILENAME(NODE) LOCATION_FILE (EXPR_CHECK ((NODE))->exp.locus)
 #define EXPR_LINENO(NODE) LOCATION_LINE (EXPR_CHECK (NODE)->exp.locus)