[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #25 from Paul Pluzhnikov 2012-06-19 21:56:31 UTC --- Google ref: b/6695435
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 Richard Guenther changed: What|Removed |Added Target Milestone|4.7.1 |4.7.2 --- Comment #24 from Richard Guenther 2012-06-14 08:10:57 UTC --- GCC 4.7.1 is being released, adjusting target milestone.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #23 from Paolo Carlini 2012-06-13 14:01:10 UTC --- Richard: http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00236.html
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #22 from Richard Guenther 2012-06-13 13:49:34 UTC --- Was this fixed?
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #21 from paolo at gcc dot gnu.org 2012-06-04 20:31:04 UTC --- Author: paolo Date: Mon Jun 4 20:30:59 2012 New Revision: 188207 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188207 Log: 2012-06-04 Paolo Carlini PR c++/53524 * doc/invoke.texi (Wenum-compare): Update documentation. /cp 2012-06-04 Paolo Carlini PR c++/53524 * call.c (build_conditional_expr_1): Use OPT_Wenum_compare to control enumeral mismatch in conditional expression too. /testsuite 2012-06-04 Paolo Carlini PR c++/53524 * g++.dg/warn/Wenum-compare-no-2: New. Added: branches/gcc-4_7-branch/gcc/testsuite/g++.dg/warn/Wenum-compare-no-2.C Modified: branches/gcc-4_7-branch/gcc/ChangeLog branches/gcc-4_7-branch/gcc/cp/ChangeLog branches/gcc-4_7-branch/gcc/cp/call.c branches/gcc-4_7-branch/gcc/doc/invoke.texi branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #20 from paolo at gcc dot gnu.org 2012-06-04 19:27:26 UTC --- Author: paolo Date: Mon Jun 4 19:27:12 2012 New Revision: 188204 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188204 Log: 2012-06-04 Paolo Carlini PR c++/53524 * doc/invoke.texi (Wenum-compare): Update documentation. /cp 2012-06-04 Paolo Carlini PR c++/53524 * call.c (build_conditional_expr_1): Use OPT_Wenum_compare to control enumeral mismatch in conditional expression too. /testsuite 2012-06-04 Paolo Carlini PR c++/53524 * g++.dg/warn/Wenum-compare-no-2: New. Added: trunk/gcc/testsuite/g++.dg/warn/Wenum-compare-no-2.C Modified: trunk/gcc/ChangeLog trunk/gcc/cp/ChangeLog trunk/gcc/cp/call.c trunk/gcc/doc/invoke.texi trunk/gcc/testsuite/ChangeLog
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
--- Comment #19 from Paul Pluzhnikov 2012-05-31
21:33:51 UTC ---
(In reply to comment #16)
> In any case I recommend not mixing together here different issues:
I don't believe there are separate issues here.
> the first
> one, subject of this report, is overeager warning in open enum context with
> conditional operators
Yes, that warning is overeager *in that context*, i.e. it fires on correct
code. Further, that code is quite common in template metaprogramming,
and we've had it fire in several different packages.
> (I understand that by now we agree that the warning is
> behaving as designed,
I don't believe we agree on that.
The warning itself is good (behaving as designed) in this context:
enum { A };
enum { B };
return 0 ? A : B; // bug
I wouldn't want to suppress that warning above.
But I would *have* to suppress it (we build with -Werror), if it also fires
in the open enum context, where it does *not* behave as designed.
And that would be quite undesireable.
> only it seems sensible to have a way to at least
> controlling it, cmp C front end, EDG); the second one is about speculative
> defects in the way the C++ front end handles enumerated types. For the latter
> we need testcases in Bugzilla, actually a separate PR should be opened.
The second issue you are referring to is really just an explanation of
why the code that generates the warning is doing so: it has been given
bad data.
In comment#15, Lawrence suggested an approach ("Second, ...") that would
avoid giving bad data to build_conditional_expr_1().
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 Paolo Carlini changed: What|Removed |Added AssignedTo|paolo.carlini at oracle dot |unassigned at gcc dot |com |gnu.org --- Comment #18 from Paolo Carlini 2012-05-31 21:17:29 UTC --- Really? If that's the case, I'm learning that now, then I don't think I can help.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #17 from Lawrence Crowl 2012-05-31 21:08:51 UTC --- > In any case I recommend not mixing together here different issues: > the first one, subject of this report, is overeager warning in open > enum context with conditional operators (I understand that by now > we agree that the warning is behaving as designed, only it seems > sensible to have a way to at least controlling it, cmp C front end, > EDG); the second one is about speculative defects in the way the > C++ front end handles enumerated types. For the latter we need > testcases in Bugzilla, actually a separate PR should be opened. I think this bug report is on how the C++ front end handles enums. Its test case is very focussed on the problem. The other test cases WRT this error message seem to be working correctly. So, I think any new PR should be an RFE for the warning option.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #16 from Paolo Carlini 2012-05-31 17:48:06 UTC --- In any case I recommend not mixing together here different issues: the first one, subject of this report, is overeager warning in open enum context with conditional operators (I understand that by now we agree that the warning is behaving as designed, only it seems sensible to have a way to at least controlling it, cmp C front end, EDG); the second one is about speculative defects in the way the C++ front end handles enumerated types. For the latter we need testcases in Bugzilla, actually a separate PR should be opened.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #15 from Lawrence Crowl 2012-05-31 17:18:49 UTC --- > When we emit the warning, arg2_type and arg3_type are the types > of arg2 and arg3, thus, post PR16603, exactly the types of the > two initializing expressions, because we are still defining the > enumerator NumLowBitsAvailable of the same enum and the enum > is not complete. And indeed, those types are *different* as > the warning says. The types of PointerLikeTypeTraits < PT1 >::NumLowBitsAvailable and PointerLikeTypeTraits < PT2 >::NumLowBitsAvailable are enums, but the types of PT1BitsAv and PT2BitsAv are the underlying type, some form of int. The normal integral promotion should provide the ?: result type. That is, we cannot just copy the type of the initializer for PT1BitsAv, we need to determine the underlying type corresponding to the converted value. > Thus, it seems to me, the warning is behaving as designed, just, > post PR16603, it triggers also while we are defining individual > enumerators basing on other enumerators of the same enum. I agree that the basic warning is right. I am not convinced that the enumerator types are handled as per the standard. > Of course this didn't happen before PR16603 because we weren't > honoring the two-phase typing mechanism. Then it seems to me that > we have nothing to strictly-speaking "fix", but only to agree > on how we want to put the warning under control. I'm tempted to > propose again just to add a -Wenum-mismatch, I note that the EDG > front-end doesn't warn with -Wall for the reference very simple > case discussed in Comments #7 and #8. I *do* understand that > ideally we would like to tell the code in build_conditional_expr_1: > "hey we are comparing the types of the initializing expressions > of two enumerators of the same enum, which are different, but the > difference will go away at the end of the enum when we'll have a > single underlying type, thus please don't warn now" but I don't > see a simple way to do this: if, for example, we just compare > underlying types, we suppress a lot of other warnings, like the > one in Comment #7. Given what I see for EDG (what about CLANG?), > I'm not sure we should spend a lot of time right now tuning the > mechanism of the warning itself. I see two approaches. First as you suggest above, which requires passing context into the comparison. Second, set each enumerator type at the point of its definition to its unique underlying type, and at the end of the enumeration, update all the enumerators to have the enumeration's underlying type.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 Paolo Carlini changed: What|Removed |Added CC||jason at gcc dot gnu.org --- Comment #14 from Paolo Carlini 2012-05-31 00:58:14 UTC --- I'm reviewing the whole thing. To summarize my understanding: When we emit the warning, arg2_type and arg3_type are the types of arg2 and arg3, thus, post PR16603, exactly the types of the two initializing expressions, because we are still defining the enumerator NumLowBitsAvailable of the same enum and the enum is not complete. And indeed, those types are *different* as the warning says. Thus, it seems to me, the warning is behaving as designed, just, post PR16603, it triggers also while we are defining individual enumerators basing on other enumerators of the same enum. Of course this didn't happen before PR16603 because we weren't honoring the two-phase typing mechanism. Then it seems to me that we have nothing to strictly-speaking "fix", but only to agree on how we want to put the warning under control. I'm tempted to propose again just to add a -Wenum-mismatch, I note that the EDG front-end doesn't warn with -Wall for the reference very simple case discussed in Comments #7 and #8. I *do* understand that ideally we would like to tell the code in build_conditional_expr_1: "hey we are comparing the types of the initializing expressions of two enumerators of the same enum, which are different, but the difference will go away at the end of the enum when we'll have a single underlying type, thus please don't warn now" but I don't see a simple way to do this: if, for example, we just compare underlying types, we suppress a lot of other warnings, like the one in Comment #7. Given what I see for EDG (what about CLANG?), I'm not sure we should spend a lot of time right now tuning the mechanism of the warning itself.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #13 from Paolo Carlini 2012-05-30 17:55:07 UTC --- Thanks Manuel and Lawrence. If I understand correctly what L said, the simpler and more urgent thing to do is making the warning code a bit smarter, I'll try to do that.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
--- Comment #12 from Manuel López-Ibáñez 2012-05-30
17:43:38 UTC ---
(In reply to comment #9)
> I think there is a largely linguistic misunderstanding: when I said unintended
> I meant that I did not *anticipate* that after my patch, which was fixing a
> real bug, we would end up warning more, in templates too. For sure the
> testcase
> I mentioned in my last message pre existed and likewise the code I posted
> here.
> In my opinion the warning, by itself, definitely makes sense in general, but
> it
> looks like, we may not want it uncinditionally, we may want to control it,
> otherwise can be annoying in the template contexts mentioned here. Do you
> agree
> with my summary? In practice: shall we give the warning a name and a switch?
Well, personally I think all warnings should have a switch, -Wenum-mismatch ?
But I don't see why the fact that it is triggered within a template makes a
difference:
enum { e1 };
enum { e2 };
enum {
a1 = e1,
a2 = e2,
a3 = 0 ? a1 : a2,
a4 = 0 ? e1 : e2,
};
int j;
void foo(void) {
j = a3;
}
I think what we don't want to warn for a3 but we want to warn for a4 because we
assume that a1 and a2 have the same type (despite the standard seems to say
that they don't until the closing brace) or we assume that the user does not
care.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
--- Comment #11 from Lawrence Crowl 2012-05-30
17:42:14 UTC ---
(In reply to comment #7)
> Let's add in CC Gaby, in the testsuite I see the warning triggering outside
> templates for a testcase coming from a bug report of him,
> g++.old-deja/g++.other/cond5.C, we have:
>
> enum E1 {e1 = -1};
> enum E2 {e2 = -1};
>
> int j;
>
> j = (i ? e1 : e2);// { dg-warning "mismatch" }
>
> Shall we not warn by default? Shall we give the warning a name (which?) and
> add
> it to -Wall? To -Wextra? Neither?
I believe this testcase is different and the warning is correct.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
--- Comment #10 from Lawrence Crowl 2012-05-30
17:37:37 UTC ---
(In reply to comment #6)
> The warnings are an unintended effect of my fix for PR16603.
>
> We warn at the end of the below lines of call.c. At the moment isn't clear to
> me *when* it would actually make sense to warn. Hints?
>
> ///
>
> /* [expr.cond]
>
> After those conversions, one of the following shall hold:
>
> --The second and third operands have the same type; the result is of
>that type. */
> if (same_type_p (arg2_type, arg3_type))
> result_type = arg2_type;
> /* [expr.cond]
>
> --The second and third operands have arithmetic or enumeration
>type; the usual arithmetic conversions are performed to bring
>them to a common type, and the result is of that type. */
> else if ((ARITHMETIC_TYPE_P (arg2_type)
> || UNSCOPED_ENUM_P (arg2_type))
>&& (ARITHMETIC_TYPE_P (arg3_type)
>|| UNSCOPED_ENUM_P (arg3_type)))
> {
> /* In this case, there is always a common type. */
> result_type = type_after_usual_arithmetic_conversions (arg2_type,
> arg3_type);
> do_warn_double_promotion (result_type, arg2_type, arg3_type,
> "implicit conversion from %qT to %qT to "
> "match other result of conditional",
> input_location);
>
> if (TREE_CODE (arg2_type) == ENUMERAL_TYPE
> && TREE_CODE (arg3_type) == ENUMERAL_TYPE)
> {
> if (complain & tf_warning)
> warning (0,
> "enumeral mismatch in conditional expression: %qT vs
> %qT",
> arg2_type, arg3_type);
> }
In order to get this error, both arg2 and arg3 must be interpreted as enum
types. However, in the testcase, we are still in the enum definition, and
we are referring to enumerators in that definition, so at that point the
types of the arguments should be some form of int. That is, the compiler
should either change the types of the enumerators at the closing brace, or
look through the enumerator's enum type to its underlying type while in
the definition that encloses the enumerator.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #9 from Paolo Carlini 2012-05-30 17:22:45 UTC --- I think there is a largely linguistic misunderstanding: when I said unintended I meant that I did not *anticipate* that after my patch, which was fixing a real bug, we would end up warning more, in templates too. For sure the testcase I mentioned in my last message pre existed and likewise the code I posted here. In my opinion the warning, by itself, definitely makes sense in general, but it looks like, we may not want it uncinditionally, we may want to control it, otherwise can be annoying in the template contexts mentioned here. Do you agree with my summary? In practice: shall we give the warning a name and a switch?
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
--- Comment #8 from Manuel López-Ibáñez 2012-05-30
16:29:08 UTC ---
(In reply to comment #6)
> The warnings are an unintended effect of my fix for PR16603.
So, before your fix, same_type_p was returning true?
enum { e1 = -1 };
enum { e2 = -1 };
int j;
void foo(void) {
j = 0 ? e1 : e2;
}
g++ 4.3.2
tt.cc:6: warning: enumeral mismatch in conditional expression: ‘’ vs ‘’
So, the warning is intended, and the fact that we did not warn before was a
bug. However, it is not clear to me how this code can be dangerous. But the
warning is very very old.
r29056 | nathan | 1999-09-02 11:21:42 +0200 (Thu, 02 Sep 1999) | 4 lines
* call.c (build_conditional_expr): Warn on enum mismatches.
(convert_arg_to_ellipsis): Move non-pod check to after
conversion.
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
Paolo Carlini changed:
What|Removed |Added
CC||g...@integrable-solutions.ne
||t
--- Comment #7 from Paolo Carlini 2012-05-30
11:06:20 UTC ---
Let's add in CC Gaby, in the testsuite I see the warning triggering outside
templates for a testcase coming from a bug report of him,
g++.old-deja/g++.other/cond5.C, we have:
enum E1 {e1 = -1};
enum E2 {e2 = -1};
int j;
j = (i ? e1 : e2);// { dg-warning "mismatch" }
Shall we not warn by default? Shall we give the warning a name (which?) and add
it to -Wall? To -Wextra? Neither?
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524
Paolo Carlini changed:
What|Removed |Added
AssignedTo|unassigned at gcc dot |paolo.carlini at oracle dot
|gnu.org |com
--- Comment #6 from Paolo Carlini 2012-05-30
10:22:33 UTC ---
The warnings are an unintended effect of my fix for PR16603.
We warn at the end of the below lines of call.c. At the moment isn't clear to
me *when* it would actually make sense to warn. Hints?
///
/* [expr.cond]
After those conversions, one of the following shall hold:
--The second and third operands have the same type; the result is of
that type. */
if (same_type_p (arg2_type, arg3_type))
result_type = arg2_type;
/* [expr.cond]
--The second and third operands have arithmetic or enumeration
type; the usual arithmetic conversions are performed to bring
them to a common type, and the result is of that type. */
else if ((ARITHMETIC_TYPE_P (arg2_type)
|| UNSCOPED_ENUM_P (arg2_type))
&& (ARITHMETIC_TYPE_P (arg3_type)
|| UNSCOPED_ENUM_P (arg3_type)))
{
/* In this case, there is always a common type. */
result_type = type_after_usual_arithmetic_conversions (arg2_type,
arg3_type);
do_warn_double_promotion (result_type, arg2_type, arg3_type,
"implicit conversion from %qT to %qT to "
"match other result of conditional",
input_location);
if (TREE_CODE (arg2_type) == ENUMERAL_TYPE
&& TREE_CODE (arg3_type) == ENUMERAL_TYPE)
{
if (complain & tf_warning)
warning (0,
"enumeral mismatch in conditional expression: %qT vs %qT",
arg2_type, arg3_type);
}
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 Richard Guenther changed: What|Removed |Added Keywords||diagnostic Version|unknown |4.7.1 Target Milestone|--- |4.7.1
[Bug c++/53524] [4.7/4.8 Regression] Bogus and unsuppressible enum comparison warning
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53524 --- Comment #5 from Paolo Carlini 2012-05-30 08:52:09 UTC --- Note that in any case in current mainline the location is exactly the '?' of the conditional expression: maybe the error message doesn't make sense but lately we are making progress on the locations ;)
