Bug 12242 - g++ should warn about out-of-range int->enum conversions
g++ should warn about out-of-range int->enum conversions
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: c++
3.4.0
: P3 enhancement
: 4.4.0
Assigned To: Manuel López-Ibáñez
: diagnostic
: 11749 (view as bug list)
Depends on:
Blocks: Wundefined
  Show dependency treegraph
 
Reported: 2003-09-10 22:28 UTC by snyder
Modified: 2008-08-09 00:35 UTC (History)
9 users (show)

See Also:
Host: i686-pc-linux-gnu
Target: i686-pc-linux-gnu
Build: i686-pc-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2006-01-23 01:20:08


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description snyder 2003-09-10 22:28:04 UTC
This note concerns conversions from an integral constant to an enum,
where the constant is out of the valid range for the enum.  I.e.,

enum E { A };
...
E e = static_cast<E> (10);

Yes, i know that result of this is undefined.

However, previous versions of g++ (and every other c++ compiler i've used)
has implemented this in what seems like the simplest way: the compiler
simply assigns the value `10' to the enum value, regardless of the
fact that this is not a valid value for the enum.

Within the past month, however, the behavior of g++ has changed; it
will now alter the value assigned to the enum so that it is within
the valid range of the enum.  This can be seen with the example
program below.  If i run it with a recent cvs version of 3.4, it
prints `0'.  If i use an older version of g++, it prints `10'.

As i mentioned before, the result of this is explicitly undefined
in the standard, so either behavior is perfectly legitimate.
However, if someone does write code like the above, it is likely
that that the intent is to actually assign the invalid value
to the enum.  Besides being the simplest interpretation of what
is meant, this is reinforced by the fact that other compilers
and older g++ versions do indeed interpret it this way.

Therefore, the compiler is silently changing the behavior of the
code from what was likely intended.  So i think a warning would
be appropriate in this situation.  The current verison of 3.4 does
not issue any warning, even with `-Wall -pedantic'.

[I ran into this because i had a piece of code taking an enum as an
input that was checking that the enum was in a valid range as part
of validating its inputs.  The unit test for this piece of code
was then deliberately trying to call it with an invalid enum in order to
exercise that code path.]

Environment:
System: Linux karma 2.4.19-emp_2419p5a829i #1 Tue Sep 3 17:42:17 EST 2002 i686 i686 i386 GNU/Linux
Architecture: i686

	
host: i686-pc-linux-gnu
build: i686-pc-linux-gnu
target: i686-pc-linux-gnu
configured with: ../gcc/configure --prefix=/usr/local/gcc --enable-threads=posix --enable-long-long --enable-languages=c,c++,f77

How-To-Repeat:

------------------------
extern "C" int printf(...);

enum E { A };

void bar (E e) { printf ("%d\n", (int)e);}

int main()
{
  bar (static_cast<E>(10));
  return 0;
}
------------------------
Comment 1 Andrew Pinski 2003-09-10 22:30:47 UTC
This is a dup of bug 11749.

*** This bug has been marked as a duplicate of 11749 ***
Comment 2 Wolfgang Bangerth 2003-09-10 23:10:09 UTC
Let's turn this around. This PR has much more information than the other one 
so let's keep it open and redirect the other one. 
 
W. 
Comment 3 Wolfgang Bangerth 2003-09-10 23:10:38 UTC
Legitimate, so confirmed. 
Comment 4 Wolfgang Bangerth 2003-09-10 23:11:27 UTC
*** Bug 11749 has been marked as a duplicate of this bug. ***
Comment 5 Andrew Pinski 2004-08-13 23:45:46 UTC
*** Bug 17022 has been marked as a duplicate of this bug. ***
Comment 6 Gabriel Dos Reis 2006-01-23 01:20:08 UTC
Working on a patch.
Comment 7 Manuel López-Ibáñez 2006-07-08 18:33:48 UTC
(In reply to comment #6)
> Working on a patch.
> 

Hi Gabriel, what is your patch going to do? Is it going to emit a warning? Would it be appropriate to emit the same warning for the C front end?
Comment 8 Manuel López-Ibáñez 2007-01-21 17:47:55 UTC
I was thinking about adding this to Wconversion. But perhaps it is more appropriate for Wundefined. After all, the value may change or not depending on the particular implementation, since it is undefined. 

Gabriel, should I add it to the list of Wundefined?

Comment 9 Manuel López-Ibáñez 2008-02-15 11:09:28 UTC
(In reply to comment #0)
> enum E { A };
> ...
> E e = static_cast<E> (10);
> 
> Yes, i know that result of this is undefined.

Is this true? I haven't found any mention of this.

> However, previous versions of g++ (and every other c++ compiler i've used)
> has implemented this in what seems like the simplest way: the compiler
> simply assigns the value `10' to the enum value, regardless of the
> fact that this is not a valid value for the enum.

We assing 10 in GCC 4.3.

> Therefore, the compiler is silently changing the behavior of the
> code from what was likely intended.  So i think a warning would
> be appropriate in this situation.  The current verison of 3.4 does
> not issue any warning, even with `-Wall -pedantic'.

The following patch implements the warning for this testcase. Do you know other cases that may be interesting to warn about?

Index: gcc/testsuite/g++.dg/warn/pr12242.C
===================================================================
--- gcc/testsuite/g++.dg/warn/pr12242.C (revision 0)
+++ gcc/testsuite/g++.dg/warn/pr12242.C (revision 0)
@@ -0,0 +1,4 @@
+// { dg-do compile }
+// { dg-options "-Wall -Wextra -Wconversion" }
+enum E { A };
+E e = static_cast<E> (10); // { dg-warning "warning.*undefined" }
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c     (revision 132310)
+++ gcc/cp/typeck.c     (working copy)
@@ -5069,10 +5069,15 @@ build_static_cast_1 (tree type, tree exp
      types which are integral, floating, or enumeration types can be
      performed.  */
   if ((INTEGRAL_TYPE_P (type) || SCALAR_FLOAT_TYPE_P (type))
       && (INTEGRAL_TYPE_P (intype) || SCALAR_FLOAT_TYPE_P (intype)))
     {
+      if (TREE_CODE (expr) == INTEGER_CST
+         && TREE_CODE (type) == ENUMERAL_TYPE
+         && !int_fits_type_p (expr, type))
+       warning (0, "this conversion is undefined");
+
       expr = ocp_convert (type, expr, CONV_C_CAST, LOOKUP_NORMAL);

       /* Ignore any integer overflow caused by the cast.  */
       expr = ignore_overflows (expr, orig);
       return expr;
Comment 10 Andrew Pinski 2008-02-15 11:15:01 UTC
> Is this true? I haven't found any mention of this.
It is at least unspecified.
Comment 11 Manuel López-Ibáñez 2008-02-15 11:32:18 UTC
(In reply to comment #10)
> > Is this true? I haven't found any mention of this.
> It is at least unspecified.
> 

So warning makes sense then, doesn't it? But perhaps the wording is not ideal.
Comment 12 Richard Biener 2008-02-15 11:34:17 UTC
It should probably say that it will truncate the value, which is what will
happen.
Comment 13 Manuel López-Ibáñez 2008-02-15 11:42:12 UTC
(In reply to comment #12)
> It should probably say that it will truncate the value, which is what will
> happen.
> 

That doesn't happen. The result of the conversion is 10 in GCC 4.3. It seems it doesn't need to be 10 and it wasn't 10 in 3.4. I don't want to get a PR saying "the warning says that the value is truncated but I printed it and it is not truncated! the warning is wrong! what a crappy compiler!". So better say that whatever may happen in a formal way.
Comment 14 Wolfgang Bangerth 2008-08-07 05:41:04 UTC
Patch now here:
  http://gcc.gnu.org/ml/gcc-patches/2008-08/msg00436.html
Comment 15 Wolfgang Bangerth 2008-08-07 05:41:39 UTC
Hi Manu,
just saw your patch for PR 12242 and have a comment: I believe the warning message would be much better if it said *why* the result is unspecified (if the expression being cast is a bit more complicated then it may not be immediately clear to the casual observer that the reason is that the value is out of range; one could also think that for whatever reason conversions between enums are unspecified -- which of course isn't the true reason).

So I think a better wording would be
+	warning (OPT_Wconversion, 
+		 "the result of %<static_cast<%T>(%E)%> is unspecified because the " 
+               "value is out of range for type %T",
+		 type, expr, type);

Best
  Wolfgang
Comment 16 Manuel López-Ibáñez 2008-08-07 07:50:39 UTC
The expression cannot be very complicated because it needs to be a INTEGER_CST. On the other hand, I agree that it is best to give users as much information as reasonable. I think it is better if you comment in gcc-patches so reviewers can see your proposal. I would prefer to not repeat twice %T, though. Also, I think users are going to have trouble to understand that the range of the enumeral is larger than the number of enumerators it contains. Perhaps we should print the range of %T?

"the result of %<static_cast%> is unspecified because %qE is outside the range [%d,%d] of type %qT."


Comment 17 Wolfgang Bangerth 2008-08-07 13:13:23 UTC
(In reply to comment #16)
> The expression cannot be very complicated because it needs to be a INTEGER_CST.

Sure, but it can be of the form
   ~SOME_PREPROCESSOR_MACRO | (MACRO2 ^ MACRO3)
What I meant to say is that the value of the expression may not be
obvious to the author of the code.


> On the other hand, I agree that it is best to give users as much information as
> reasonable. I think it is better if you comment in gcc-patches so reviewers can
> see your proposal.

Will do next time, I guess for this time we're stuck here :-)


> I would prefer to not repeat twice %T, though. Also, I think
> users are going to have trouble to understand that the range of the enumeral is
> larger than the number of enumerators it contains. Perhaps we should print the
> range of %T?
> 
> "the result of %<static_cast%> is unspecified because %qE is outside the range
> [%d,%d] of type %qT."

That would be an excellent idea as well.

Thanks
 Wolfgang
Comment 18 Manuel López-Ibáñez 2008-08-09 00:32:09 UTC
Subject: Bug 12242

Author: manu
Date: Sat Aug  9 00:30:41 2008
New Revision: 138898

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138898
Log:
2008-08-09  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>

	PR c++/12242
cp/
	* cvt.c (ocp_convert): Warn for out-of-range conversions to enum.

testsuite/
	* g++.dg/warn/pr12242.C: New.

Added:
    trunk/gcc/testsuite/g++.dg/warn/pr12242.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cvt.c
    trunk/gcc/testsuite/ChangeLog

Comment 19 Manuel López-Ibáñez 2008-08-09 00:35:38 UTC
FIXED in GCC 4.4