Patchwork [C++,Patch/RFC] PR 54194

login
register
mail settings
Submitter Paolo Carlini
Date Oct. 8, 2012, 10:03 a.m.
Message ID <5072A4F7.6020108@oracle.com>
Download mbox | patch
Permalink /patch/189969/
State New
Headers show

Comments

Paolo Carlini - Oct. 8, 2012, 10:03 a.m.
Hi,

in this PR submitter points out that in the -Wparentheses warning, for, eg,

char in[4]={0}, out[6];
out[1] = in[1] & 0x0F | ((in[3] & 0x3C) << 2);

warning: suggest parentheses around arithmetic in operand of ‘|’ 
[-Wparentheses]

the caret points to end of the expression, ie the final closing 
parenthesis, which is rather misleading, because the problem is actually 
in the first operand of '|'. Ideally I guess one would like to somehow 
point to that first operand, but our infrastructure (shared with the C 
front-end, at the moment) isn't really ready to do that, and probably we 
would like to use a range (more than a caret) below the whole first 
operand (the problem isn't really with & per se). Considering also what 
we are already doing elsewhere, it seems to me that a straightforward 
and good improvement is obtained by passing to warn_about_parentheses 
the location of the outer operand (together with its code), as per the 
attached patchlet: then in the example the caret points to the actual 
'|' operator mentioned in the error message. Post 4.8.0 we can imagine 
further improvements...

What do you think?

Thanks,
Paolo.

///////////////////////////
Jason Merrill - Oct. 8, 2012, 1:57 p.m.
This is definitely an improvement, though for warnings about issues with 
the left or right argument, we could use the EXPR_LOCATION of the 
problematic argument rather than the location of the new operand.

Jason
Paolo Carlini - Oct. 8, 2012, 2:04 p.m.
On 10/08/2012 03:57 PM, Jason Merrill wrote:
> This is definitely an improvement, though for warnings about issues 
> with the left or right argument, we could use the EXPR_LOCATION of the 
> problematic argument rather than the location of the new operand.
I agree. Let me see if I can figure out something straightforward enough.

Thanks!
Paolo.
Paolo Carlini - Oct. 8, 2012, 9:31 p.m.
On 10/08/2012 04:04 PM, Paolo Carlini wrote:
> On 10/08/2012 03:57 PM, Jason Merrill wrote:
>> This is definitely an improvement, though for warnings about issues 
>> with the left or right argument, we could use the EXPR_LOCATION of 
>> the problematic argument rather than the location of the new operand.
> I agree. Let me see if I can figure out something straightforward enough.
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?

Thanks,
Paolo.
Jason Merrill - Oct. 8, 2012, 9:44 p.m.
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.

Jason
Paolo Carlini - Oct. 8, 2012, 10 p.m.
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.
Yes. In practice build_x_binary_op calls warn_about_parentheses and 
simply forwards the expressions passes by cp_parser_binary_expression. 
Looks like we have a latent issue with the latter not setting any 
location for left and right. Let me see...
> 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.
Indeed, I had the same idea...

Paolo.

Patch

Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 192130)
+++ cp/typeck.c	(working copy)
@@ -3630,7 +3630,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: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 192130)
+++ c-family/c-common.c	(working copy)
@@ -10428,7 +10428,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)
 {
@@ -10449,26 +10449,26 @@  void
     {
     case LSHIFT_EXPR:
       if (code_left == PLUS_EXPR || code_right == PLUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<+%> inside %<<<%>");
+	warning_at (loc, OPT_Wparentheses,
+		    "suggest parentheses around %<+%> inside %<<<%>");
       else if (code_left == MINUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<-%> inside %<<<%>");
+	warning_at (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 %<>>%>");
+	warning_at (loc, OPT_Wparentheses,
+		    "suggest parentheses around %<+%> inside %<>>%>");
       else if (code_left == MINUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
-		 "suggest parentheses around %<-%> inside %<>>%>");
+	warning_at (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 %<||%>");
+	warning_at (loc, OPT_Wparentheses,
+		    "suggest parentheses around %<&&%> within %<||%>");
       return;
 
     case BIT_IOR_EXPR:
@@ -10476,18 +10476,19 @@  void
 	  || 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,
+	warning_at (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,
+	warning_at (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 (loc, OPT_Wparentheses,
+		    "suggest parentheses around operand of "
+		    "%<!%> or change %<|%> to %<||%> or %<!%> to %<~%>");
       return;
 
     case BIT_XOR_EXPR:
@@ -10495,44 +10496,45 @@  void
 	  || code_left == PLUS_EXPR || code_left == MINUS_EXPR
 	  || code_right == BIT_AND_EXPR
 	  || code_right == PLUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
+	warning_at (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,
+	warning_at (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,
+	warning_at (loc, OPT_Wparentheses,
 		 "suggest parentheses around %<+%> in operand of %<&%>");
       else if (code_left == MINUS_EXPR || code_right == MINUS_EXPR)
-	warning (OPT_Wparentheses,
+	warning_at (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,
+	warning_at (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 (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,
+	warning_at (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,
+	warning_at (loc, OPT_Wparentheses,
 		 "suggest parentheses around comparison in operand of %<!=%>");
       return;
 
@@ -10544,8 +10546,9 @@  void
 	       || (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");
+	warning_at (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 192130)
+++ c-family/c-common.h	(working copy)
@@ -988,7 +988,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: c/c-typeck.c
===================================================================
--- c/c-typeck.c	(revision 192130)
+++ 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),