diff mbox

[C++,Patch/RFC] PR 54194

Message ID 5072A4F7.6020108@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Oct. 8, 2012, 10:03 a.m. UTC 
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.

///////////////////////////

Comments

Jason Merrill Oct. 8, 2012, 1:57 p.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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. UTC | #5 
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.
diff mbox

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),