diff mbox

[C++,/,RFC] PR 45385

Message ID 4EA148F8.40404@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Oct. 21, 2011, 10:27 a.m. UTC 
Hi,

this one is a bit subtler. It's actually a regression due to the fix for 
PR35602, which was about a bogus warning for:

struct c
{
   ~c();
   c();
};

int
main()
{
   c x[0UL][0UL] =  // { dg-bogus "warning: conversion to .long unsigned 
int. from .long int. may change the sign of the result" }
     {
     };
}

The way we did it, we added a check at the beginning of 
c-common.c:conversion_warning:

    /* If any operand is artificial, then this expression was generated
       by the compiler and we do not warn.  */
   for (i = 0; i < expr_num_operands; i++)
      {
        tree op = TREE_OPERAND (expr, i);
        if (op && DECL_P (op) && DECL_ARTIFICIAL (op))
      return;
      }

which catches the artificial (only) operand of the expr (expr is a 
BIT_NOT_EXPR and the operand a VAR_DECL).

Now, however, for testcases like PR45385, where member functions are 
involved, we easily fail to produce warnings, simply because the this 
pointer is artificial! Thus I had the idea of restricting the above 
check to the single operand case which matters for PR35602: for sure 
it's a safe change, and passes the testsuite, but I cannot exclude that 
more complex situations can occur for which the loop would avoid more 
bogus warnings... What do you think, is the change good enough for now?

Thanks,
Paolo.

////////////////////
/c-family
2011-10-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/45385
	* c-common.c (conversion_warning): Early return only if the
	only operand is DECL_ARTIFICIAL.

testsuite/
2011-10-21  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/45385
	* g++.dg/warn/Wconversion4.C: New.

Comments

Jason Merrill Oct. 21, 2011, 2:56 p.m. UTC | #1 
I think the fix for 35602 was wrong; instead of trying to suppress the 
warning, we should avoid building expressions that trip it.  In this 
case, the problem is a type mismatch in build_vec_init between 
maxindex/iterator (ptrdiff_type_node) and array_type_nelts_total 
(sizetype).  And indeed, converting (ptrdiff_t)-1 to unsigned changes 
its sign.

I think a better fix for 35602 would be to bail out of build_vec_init 
exit early if maxindex is -1.

Jason
diff mbox

Patch

Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 180288)
+++ c-family/c-common.c	(working copy)
@@ -2121,19 +2121,17 @@  unsafe_conversion_p (tree type, tree expr, bool pr
 static void
 conversion_warning (tree type, tree expr)
 {
-  int i;
-  const int expr_num_operands = TREE_OPERAND_LENGTH (expr);
   tree expr_type = TREE_TYPE (expr);
   location_t loc = EXPR_LOC_OR_HERE (expr);
 
   if (!warn_conversion && !warn_sign_conversion)
     return;
 
-  /* If any operand is artificial, then this expression was generated
-     by the compiler and we do not warn.  */
-  for (i = 0; i < expr_num_operands; i++)
+  /* If the only operand is artificial, then the expression was generated
+     by the compiler and we do not warn.  ???? */
+  if (TREE_OPERAND_LENGTH (expr) == 1)
     {
-      tree op = TREE_OPERAND (expr, i);
+      tree op = TREE_OPERAND (expr, 0);
       if (op && DECL_P (op) && DECL_ARTIFICIAL (op))
 	return;
     }
Index: testsuite/g++.dg/warn/Wconversion4.C
===================================================================
--- testsuite/g++.dg/warn/Wconversion4.C	(revision 0)
+++ testsuite/g++.dg/warn/Wconversion4.C	(revision 0)
@@ -0,0 +1,17 @@ 
+// PR c++/45385
+// { dg-options "-Wconversion" } 
+
+void foo(unsigned char);
+
+class Test
+{
+  void eval()
+  {
+    foo(bar());  // { dg-warning "may alter its value" }
+  }
+
+  unsigned int bar() const
+  {
+    return __INT_MAX__ * 2U + 1;
+  }
+};