Revised patch for compare1.c test failures
Zack Weinberg
zack@rabi.columbia.edu
Thu Dec 23 10:15:00 GMT 1999
Here is the third revision of the revised patch. It bootstraps and adds no
testsuite regressions on ix86-linux. The aforementioned abort with an
IDENTIFIER_NODE in expand_expr was caused by an = vs. == mistake which took
forever to find; I have no idea why I didn't get a warning...
It adds another category of suppressed warnings: if comparing a signed value
with an unsigned enumeration constant, and the maximum value of the
enumeration constant fits in the signed type, no warning. This happens e.g.
in a for loop over all values of RID_* - see ch/grant.c for an example.
With this patch and a chaser for cp/cp-tree.h we are down to 16 instances of
"signed and unsigned type in conditional expression", and 8 instances of
"comparison between signed and unsigned".
zw
* c-decl.c (finish_enum): Simplify code to determine minimum and
maximum values of the enum, and calculate the type. Remove check
for FUNCTION_DECLs in the values list, which cannot happen. Replace
the DECL_INITIAL of each enumeration constant with a copy converted
to the enumeration type. When updating variant types, don't bother
updating the type itself.
* c-typeck.c (build_binary_op): Simplify conditional expressions
when weeding out spurious signed-unsigned warnings. Add new
spurious warning category: if the unsigned quantity is an enum
and its maximum value fits in signed_type(result_type). Update
commentary.
(build_conditional_expr): Warn here if one alternative is signed
and the other is unsigned.
===================================================================
Index: c-decl.c
--- c-decl.c 1999/12/04 03:00:02 1.91
+++ c-decl.c 1999/12/23 17:57:03
@@ -5664,8 +5664,8 @@ finish_enum (enumtype, values, attribute
{
register tree pair, tem;
tree minnode = 0, maxnode = 0;
- int lowprec, highprec, precision;
- int toplevel = global_binding_level == current_binding_level;
+ int precision, unsign;
+ int toplevel = (global_binding_level == current_binding_level);
if (in_parm_level_p ())
warning ("enum defined inside parms");
@@ -5677,67 +5677,62 @@ finish_enum (enumtype, values, attribute
if (values == error_mark_node)
minnode = maxnode = integer_zero_node;
else
- for (pair = values; pair; pair = TREE_CHAIN (pair))
- {
- tree value = TREE_VALUE (pair);
- if (pair == values)
- minnode = maxnode = TREE_VALUE (pair);
- else
- {
- if (tree_int_cst_lt (maxnode, value))
- maxnode = value;
- if (tree_int_cst_lt (value, minnode))
- minnode = value;
- }
- }
-
- TYPE_MIN_VALUE (enumtype) = minnode;
- TYPE_MAX_VALUE (enumtype) = maxnode;
-
- /* An enum can have some negative values; then it is signed. */
- TREE_UNSIGNED (enumtype) = tree_int_cst_sgn (minnode) >= 0;
-
- /* Determine the precision this type needs. */
-
- lowprec = min_precision (minnode, TREE_UNSIGNED (enumtype));
- highprec = min_precision (maxnode, TREE_UNSIGNED (enumtype));
- precision = MAX (lowprec, highprec);
-
- if (TYPE_PACKED (enumtype) || precision > TYPE_PRECISION (integer_type_node))
{
- tree narrowest = type_for_size (precision, 1);
- if (narrowest == 0)
+ minnode = maxnode = TREE_VALUE (values);
+ for (pair = TREE_CHAIN (values); pair; pair = TREE_CHAIN (pair))
{
- warning ("enumeration values exceed range of largest integer");
- narrowest = long_long_integer_type_node;
+ tree value = TREE_VALUE (pair);
+ if (tree_int_cst_lt (maxnode, value))
+ maxnode = value;
+ if (tree_int_cst_lt (value, minnode))
+ minnode = value;
}
+ }
- TYPE_PRECISION (enumtype) = TYPE_PRECISION (narrowest);
+ /* Construct the final type of this enumeration. It is the same
+ as one of the integral types - the narrowest one that fits, except
+ that normally we only go as narrow as int - and signed iff any of
+ the values are negative. */
+ unsign = (tree_int_cst_sgn (minnode) >= 0);
+ precision = MAX (min_precision (minnode, unsign),
+ min_precision (maxnode, unsign));
+ if (!TYPE_PACKED (enumtype))
+ precision = MAX (precision, TYPE_PRECISION (integer_type_node));
+ if (type_for_size (precision, unsign) == 0)
+ {
+ warning ("enumeration values exceed range of largest integer");
+ precision = TYPE_PRECISION (long_long_integer_type_node);
}
- else
- TYPE_PRECISION (enumtype) = TYPE_PRECISION (integer_type_node);
+ TYPE_MIN_VALUE (enumtype) = minnode;
+ TYPE_MAX_VALUE (enumtype) = maxnode;
+ TYPE_PRECISION (enumtype) = precision;
+ TREE_UNSIGNED (enumtype) = unsign;
TYPE_SIZE (enumtype) = 0;
layout_type (enumtype);
if (values != error_mark_node)
{
- /* Change the type of the enumerators to be the enum type.
- Formerly this was done only for enums that fit in an int,
- but the comment said it was done only for enums wider than int.
- It seems necessary to do this for wide enums,
- and best not to change what's done for ordinary narrower ones. */
+ /* Change the type of the enumerators to be the enum type. We
+ need to do this irrespective of the size of the enum, for
+ proper type checking. Replace the DECL_INITIALs of the
+ enumerators, and the value slots of the list, with copies
+ that have the enum type; they cannot be modified in place
+ because they may be shared (e.g. integer_zero_node) Finally,
+ change the purpose slots to point to the names of the decls. */
for (pair = values; pair; pair = TREE_CHAIN (pair))
{
- TREE_TYPE (TREE_PURPOSE (pair)) = enumtype;
- DECL_SIZE (TREE_PURPOSE (pair)) = TYPE_SIZE (enumtype);
- if (TREE_CODE (TREE_PURPOSE (pair)) != FUNCTION_DECL)
- DECL_ALIGN (TREE_PURPOSE (pair)) = TYPE_ALIGN (enumtype);
- }
+ tree enu = TREE_PURPOSE (pair);
- /* Replace the decl nodes in VALUES with their names. */
- for (pair = values; pair; pair = TREE_CHAIN (pair))
- TREE_PURPOSE (pair) = DECL_NAME (TREE_PURPOSE (pair));
+ TREE_TYPE (enu) = enumtype;
+ DECL_SIZE (enu) = TYPE_SIZE (enumtype);
+ DECL_ALIGN (enu) = TYPE_ALIGN (enumtype);
+ DECL_MODE (enu) = TYPE_MODE (enumtype);
+ DECL_INITIAL (enu) = convert (enumtype, DECL_INITIAL (enu));
+
+ TREE_PURPOSE (pair) = DECL_NAME (enu);
+ TREE_VALUE (pair) = DECL_INITIAL (enu);
+ }
TYPE_VALUES (enumtype) = values;
}
@@ -5745,6 +5740,8 @@ finish_enum (enumtype, values, attribute
/* Fix up all variant types of this enum type. */
for (tem = TYPE_MAIN_VARIANT (enumtype); tem; tem = TYPE_NEXT_VARIANT (tem))
{
+ if (tem == enumtype)
+ continue;
TYPE_VALUES (tem) = TYPE_VALUES (enumtype);
TYPE_MIN_VALUE (tem) = TYPE_MIN_VALUE (enumtype);
TYPE_MAX_VALUE (tem) = TYPE_MAX_VALUE (enumtype);
===================================================================
Index: c-typeck.c
--- c-typeck.c 1999/12/19 04:31:06 1.44
+++ c-typeck.c 1999/12/23 17:57:04
@@ -2390,8 +2390,6 @@ build_binary_op (code, orig_op0, orig_op
tree primop0 = get_narrower (op0, &unsignedp0);
tree primop1 = get_narrower (op1, &unsignedp1);
- /* Avoid spurious warnings for comparison with enumerators. */
-
xop0 = orig_op0;
xop1 = orig_op1;
STRIP_TYPE_NOPS (xop0);
@@ -2407,28 +2405,41 @@ build_binary_op (code, orig_op0, orig_op
all the values of the unsigned type. */
if (! TREE_UNSIGNED (result_type))
/* OK */;
- /* Do not warn if both operands are unsigned. */
+ /* Do not warn if both operands are the same signedness. */
else if (op0_signed == op1_signed)
/* OK */;
- /* Do not warn if the signed quantity is an unsuffixed
- integer literal (or some static constant expression
- involving such literals) and it is non-negative. */
- else if ((op0_signed && TREE_CODE (xop0) == INTEGER_CST
- && tree_int_cst_sgn (xop0) >= 0)
- || (op1_signed && TREE_CODE (xop1) == INTEGER_CST
- && tree_int_cst_sgn (xop1) >= 0))
- /* OK */;
- /* Do not warn if the comparison is an equality operation,
- the unsigned quantity is an integral constant and it does
- not use the most significant bit of result_type. */
- else if ((resultcode == EQ_EXPR || resultcode == NE_EXPR)
- && ((op0_signed && TREE_CODE (xop1) == INTEGER_CST
- && int_fits_type_p (xop1, signed_type (result_type)))
- || (op1_signed && TREE_CODE (xop0) == INTEGER_CST
- && int_fits_type_p (xop0, signed_type (result_type)))))
- /* OK */;
else
- warning ("comparison between signed and unsigned");
+ {
+ tree sop, uop;
+ if (op0_signed)
+ sop = xop0, uop = xop1;
+ else
+ sop = xop1, uop = xop0;
+
+ /* Do not warn if the signed quantity is an unsuffixed
+ integer literal (or some static constant expression
+ involving such literals) and it is non-negative. */
+ if (TREE_CODE (sop) == INTEGER_CST
+ && tree_int_cst_sgn (sop) >= 0)
+ /* OK */;
+ /* Do not warn if the comparison is an equality operation,
+ the unsigned quantity is an integral constant, and it
+ would fit in the result if the result were signed. */
+ else if (TREE_CODE (uop) == INTEGER_CST
+ && (resultcode == EQ_EXPR || resultcode == NE_EXPR)
+ && int_fits_type_p (uop, signed_type (result_type)))
+ /* OK */;
+ /* Do not warn if the unsigned quantity is an enumeration
+ constant and its maximum value would fit in the result
+ if the result were signed. */
+ else if (TREE_CODE (uop) == INTEGER_CST
+ && TREE_CODE (TREE_TYPE (uop)) == ENUMERAL_TYPE
+ && int_fits_type_p (TYPE_MAX_VALUE (TREE_TYPE(uop)),
+ signed_type (result_type)))
+ /* OK */;
+ else
+ warning ("comparison between signed and unsigned");
+ }
/* Warn if two unsigned values are being compared in a size
larger than their original size, and one (and only one) is the
@@ -3362,6 +3373,37 @@ build_conditional_expr (ifexp, op1, op2)
&& (code2 == INTEGER_TYPE || code2 == REAL_TYPE))
{
result_type = common_type (type1, type2);
+
+ /* If -Wsign-compare, warn here if type1 and type2 have
+ different signedness. We'll promote the signed to unsigned
+ and later code won't know it used to be different.
+ Do this check on the original types, so that explicit casts
+ will be considered, but default promotions won't. */
+ if ((warn_sign_compare < 0 ? extra_warnings : warn_sign_compare)
+ && !skip_evaluation)
+ {
+ int unsigned_op1 = TREE_UNSIGNED (TREE_TYPE (orig_op1));
+ int unsigned_op2 = TREE_UNSIGNED (TREE_TYPE (orig_op2));
+
+ if (unsigned_op1 ^ unsigned_op2)
+ {
+ /* Do not warn if the result type is signed, since the
+ signed type will only be chosen if it can represent
+ all the values of the unsigned type. */
+ if (! TREE_UNSIGNED (result_type))
+ /* OK */;
+ /* Do not warn if the signed quantity is an unsuffixed
+ integer literal (or some static constant expression
+ involving such literals) and it is non-negative. */
+ else if ((unsigned_op2 && TREE_CODE (op1) == INTEGER_CST
+ && tree_int_cst_sgn (op1) >= 0)
+ || (unsigned_op1 && TREE_CODE (op2) == INTEGER_CST
+ && tree_int_cst_sgn (op2) >= 0))
+ /* OK */;
+ else
+ warning ("signed and unsigned type in conditional expression");
+ }
+ }
}
else if (code1 == VOID_TYPE || code2 == VOID_TYPE)
{
===================================================================
Index: testsuite/gcc.dg/compare1.c
--- testsuite/gcc.dg/compare1.c 1999/09/04 02:34:10 1.2
+++ testsuite/gcc.dg/compare1.c 1999/12/23 18:08:52
@@ -4,23 +4,36 @@
/* { dg-do compile } */
/* { dg-options "-Wsign-compare" } */
-int target_flags = 1;
+int tf = 1;
-enum machine_mode
+/* This enumeration has an explicit negative value and is therefore signed. */
+enum mm1
{
- VOIDmode , PQImode , QImode , PHImode , HImode ,
- PSImode , SImode , PDImode , DImode , TImode , OImode , QFmode ,
- HFmode , TQFmode , SFmode , DFmode , XFmode , TFmode , QCmode ,
- HCmode , SCmode , DCmode , XCmode , TCmode , CQImode , CHImode ,
- CSImode , CDImode , CTImode , COImode , BLKmode , CCmode , CCXmode,
- CC_NOOVmode, CCX_NOOVmode, CCFPmode, CCFPEmode , MAX_MACHINE_MODE
+ VOID, SI, DI, MAX = -1
};
-#define Pmode ( target_flags ? DImode : SImode )
+/* This enumeration fits entirely in a signed int, but is unsigned anyway. */
+enum mm2
+{
+ VOID2, SI2, DI2, MAX2
+};
+
+int f(enum mm1 x)
+{
+ return x == (tf?DI:SI); /* { dg-bogus "signed and unsigned" "case 1" } */
+}
-int main()
+int g(enum mm1 x)
{
- enum machine_mode mode = DImode;
+ return x == (tf?DI:-1); /* { dg-bogus "signed and unsigned" "case 2" } */
+}
- return (mode == Pmode); /* { dg-bogus "warning:" "comparison between signed and unsigned" } */
+int h(enum mm2 x)
+{
+ return x == (tf?DI2:SI2); /* { dg-bogus "signed and unsigned" "case 3" } */
+}
+
+int i(enum mm2 x)
+{
+ return x == (tf?DI2:-1); /* { dg-warning "signed and unsigned" "case 4" } */
}
More information about the Gcc-patches
mailing list