| Summary: | g++ should warn about out-of-range int->enum conversions | ||
|---|---|---|---|
| Product: | gcc | Reporter: | snyder |
| Component: | c++ | Assignee: | Manuel López-Ibáñez <manu> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | bangerth, fang, gcc-bugs, gdr, igodard, manu, mueller, pawel_sikora, pinskia |
| Priority: | P3 | Keywords: | diagnostic |
| Version: | 3.4.0 | ||
| Target Milestone: | 4.4.0 | ||
| See Also: |
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30357 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87950 |
||
| 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 | |
| Bug Depends on: | |||
| Bug Blocks: | 30334 | ||
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. Legitimate, so confirmed. *** Bug 11749 has been marked as a duplicate of this bug. *** *** Bug 17022 has been marked as a duplicate of this bug. *** Working on a patch. (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? 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? (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; > Is this true? I haven't found any mention of this.
It is at least unspecified.
(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. It should probably say that it will truncate the value, which is what will happen. (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. Patch now here: http://gcc.gnu.org/ml/gcc-patches/2008-08/msg00436.html 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 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." (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 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 FIXED in GCC 4.4 |
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; } ------------------------