Package: emacs;
Reported by: Stefan Kangas <stefankangas <at> gmail.com>
Date: Thu, 9 Jan 2025 03:58:02 UTC
Severity: wishlist
Done: Stefan Kangas <stefankangas <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Pip Cet <pipcet <at> protonmail.com> To: Paul Eggert <eggert <at> cs.ucla.edu> Cc: Gerd Moellmann <gerd <at> gnu.org>, 75451 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com> Subject: bug#75451: scratch/igc: Enable CHECK_STRUCTS Date: Tue, 14 Jan 2025 21:43:59 +0000
"Paul Eggert" <eggert <at> cs.ucla.edu> writes:
> On 2025-01-14 07:22, Pip Cet wrote:
>> The GCC options are rather strange:
>
> Yes, it seems there are so many differing opinions about enums that it's
> hard to (ahem) enumerate them all.
Indeed, but I agree with Gerd that exhaustive switches are the thing to
do, when at all possible.
>> What we want, I think, is to prohibit a switch which contains a "case
>> ENUM_VALUE:" label (where ENUM_VALUE is, well, any enum's value) from
>> being either non-exhaustive across the enum or containing a default
>> label as well.
>
> I wouldn't want to prohibit all such switches. I can think of lots of
> reasons to allow them, e.g., "enum { NAME = EXPR };" is a common way to
> name an integer constant expression.
You're right. I think it would be okay to add two exceptions:
1. If an enum contains a single value, it's not "switchable", so we
don't warn about it.
2. If an enum's values don't form a contiguous range, it's not
"switchable" either.
Together with the very useful exception already present in gcc (if we
switch on a value which is an integer constant, we don't warn about the
switch statement being non-exhaustive), I think that would work. Kind
of does, with the patch below, but it obviously needs some more work :-)
svg_css_length_to_pixels doesn't handle RSVG_UNIT_CH; draw_fringe_bitmap
doesn't handle DEFAULT_CURSOR; treesit_query_error_to_string doesn't
handle TSQueryErrorLanguage. The rest seem like false positives, at
first glance.
Of course, if it encourages people to use "if" instead of "switch", that
would usually defeat the purpose.
Pip
diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index 81ca5d34df8..00b192a05da 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -5166,7 +5166,7 @@ case_compare (splay_tree_key k1, splay_tree_key k2)
tree
c_add_case_label (location_t loc, splay_tree cases, tree cond,
- tree low_value, tree high_value, tree attrs)
+ tree low_value, tree high_value, tree original_low_value, tree original_high_value, tree attrs)
{
tree type;
tree label;
@@ -5333,7 +5333,7 @@ c_add_case_label (location_t loc, splay_tree cases, tree cond,
}
/* Add a CASE_LABEL to the statement-tree. */
- case_label = add_stmt (build_case_label (low_value, high_value, label));
+ case_label = add_stmt (build_case_label (low_value, high_value, original_low_value, original_high_value, label));
/* Register this case label in the splay tree. */
splay_tree_insert (cases,
(splay_tree_key) low_value,
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index efd942c482b..1700784beaf 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1067,6 +1067,7 @@ extern void c_init_preprocess (void);
#define ENUM_FIXED_UNDERLYING_TYPE_P(NODE) (TYPE_LANG_FLAG_5 (NODE))
extern tree do_case (location_t, tree, tree, tree);
+extern tree do_case (location_t, tree, tree, tree, tree);
extern tree build_stmt (location_t, enum tree_code, ...);
extern tree build_real_imag_expr (location_t, enum tree_code, tree);
@@ -1095,6 +1096,7 @@ extern tree boolean_increment (enum tree_code, tree);
extern int case_compare (splay_tree_key, splay_tree_key);
extern tree c_add_case_label (location_t, splay_tree, tree, tree, tree,
+ tree, tree,
tree = NULL_TREE);
extern bool c_switch_covers_all_cases_p (splay_tree, tree);
extern bool c_block_may_fallthru (const_tree);
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index d547b08f55d..94cbe203428 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -1578,6 +1578,57 @@ match_case_to_enum (splay_tree_node node, void *data)
/* Handle -Wswitch*. Called from the front end after parsing the
switch construct. */
+
+static bool
+c_switchable_enum (tree type)
+{
+ if (type == NULL_TREE)
+ return false;
+
+ if (TREE_CODE (type) != ENUMERAL_TYPE)
+ return false;
+
+ tree min_value = NULL_TREE;
+ tree max_value = NULL_TREE;
+ size_t n_values = 0;
+ for (tree chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
+ {
+ tree value = TREE_VALUE (chain);
+ tree attrs = DECL_ATTRIBUTES (value);
+ value = DECL_INITIAL (value);
+ if (!min_value || tree_int_cst_compare (value, min_value) < 0)
+ min_value = value;
+ if (!max_value || tree_int_cst_compare (value, max_value) > 0)
+ max_value = value;
+ n_values++;
+ }
+
+ /* single-value enum */
+ if (n_values < 2 || tree_int_cst_compare (min_value, max_value) == 0)
+ return false;
+
+ while (tree_int_cst_compare (min_value, max_value) <= 0)
+ {
+ bool okay = false;
+ for (tree chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
+ {
+ tree value = TREE_VALUE (chain);
+ value = DECL_INITIAL (value);
+ if (tree_int_cst_compare (value, min_value) == 0)
+ {
+ okay = true;
+ break;
+ }
+ }
+ if (!okay)
+ return false;
+
+ min_value = build_int_cst (TREE_TYPE (min_value), tree_to_shwi (min_value) + 1);
+ }
+
+ return true;
+}
+
/* ??? Should probably be somewhere generic, since other languages
besides C and C++ would want this. At the moment, however, C/C++
are the only tree-ssa languages that support enumerations at all,
@@ -1591,7 +1642,62 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
splay_tree_node node;
tree chain;
bool outside_range_p = false;
+ tree enum_type = NULL;
+ tree int_type = NULL;
+ bool inferred_type = false;
+ tree last_high_value = NULL_TREE;
+ for (splay_tree_node node = splay_tree_successor (cases, (splay_tree_key) NULL);
+ ;
+ node = splay_tree_successor (cases, (splay_tree_key) CASE_LOW ((tree)node->value)))
+ {
+ if (!node)
+ break;
+ if (node->key)
+ {
+ if (CASE_ORIGINAL_LOW ((tree) node->value))
+ {
+ tree case_type = CASE_ORIGINAL_LOW ((tree) node->value);
+ again:
+ if (TREE_CODE (case_type) == ENUMERAL_TYPE)
+ {
+ if (enum_type && enum_type != case_type)
+ {
+ location_t loc = EXPR_LOCATION ((tree) node->value);
+ warning_at (loc,
+ OPT_Wswitch_enum,
+ "two different enums used in the same switch");
+ }
+ if (int_type)
+ {
+ location_t loc = EXPR_LOCATION ((tree) node->value);
+ warning_at (loc,
+ OPT_Wswitch_enum,
+ "enums and integer values used in the same switch");
+ }
+ enum_type = case_type;
+ }
+ else if (INTEGRAL_TYPE_P (case_type))
+ {
+ if (enum_type)
+ {
+ location_t loc = EXPR_LOCATION ((tree) node->value);
+ warning_at (loc,
+ OPT_Wswitch_enum,
+ "enums and integer values used in the same switch");
+ }
+ int_type = case_type;
+ }
+ else
+ {
+ case_type = TREE_TYPE (case_type);
+ goto again;
+ }
+ }
+ }
+ if (!(splay_tree_key) CASE_LOW ((tree) node->value))
+ break;
+ }
if (type != error_mark_node
&& type != TREE_TYPE (cond)
&& INTEGRAL_TYPE_P (type)
@@ -1723,6 +1829,24 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
"switch condition has boolean value");
}
+ if (TREE_CODE (type) != ENUMERAL_TYPE)
+ {
+ if (!enum_type)
+ return;
+
+ type = enum_type;
+ inferred_type = true;
+ }
+ else
+ {
+ if (enum_type && enum_type != type)
+ {
+ location_t loc = EXPR_LOCATION (cond);
+ warning_at (loc,
+ OPT_Wswitch_enum,
+ "enum type mismatch");
+ }
+ }
/* From here on, we only care about enumerated types. */
if (!type || TREE_CODE (type) != ENUMERAL_TYPE)
return;
@@ -1746,8 +1870,15 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
O(N), since the nature of the splay tree will keep the next
element adjacent to the root at all times. */
+ bool warned = false;
for (chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
{
+ if (default_node && !warned)
+ {
+ warning_at (switch_location, OPT_Wswitch_default,
+ !inferred_type ? "enumerated switch has a default case" : "enumerated switch has a default case (inferred)");
+ warned = true;
+ }
tree value = TREE_VALUE (chain);
tree attrs = DECL_ATTRIBUTES (value);
value = DECL_INITIAL (value);
@@ -1805,12 +1936,11 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
Wswitch-enum. Otherwise, if both are enabled then we prefer
to warn using -Wswitch because -Wswitch is enabled by -Wall
while -Wswitch-enum is explicit. */
- warning_at (switch_location,
- (default_node || !warn_switch
- ? OPT_Wswitch_enum
- : OPT_Wswitch),
- "enumeration value %qE not handled in switch",
- TREE_PURPOSE (chain));
+ if (c_switchable_enum (type))
+ warning_at (switch_location,
+ OPT_Wswitch,
+ !inferred_type ? "enumeration value %qE not handled in switch" : "enumeration value %qE not handled in switch (inferred type)",
+ TREE_PURPOSE (chain));
}
/* Attribute flag_enum means bitwise combinations are OK. */
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index d2f45912cc4..eafc247f37a 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -7816,15 +7816,21 @@ c_parser_label (c_parser *parser, tree std_attrs)
if (c_parser_next_token_is_keyword (parser, RID_CASE))
{
+ struct c_expr expr;
tree exp1, exp2;
+ tree exp1o, exp2o;
c_parser_consume_token (parser);
+ expr = c_parser_expr_no_commas (parser, NULL),
exp1 = convert_lvalue_to_rvalue (loc1,
- c_parser_expr_no_commas (parser, NULL),
+ expr,
true, true).value;
+ exp1o = convert_lvalue_to_rvalue (loc1,
+ expr,
+ true, true, true).value;
if (c_parser_next_token_is (parser, CPP_COLON))
{
c_parser_consume_token (parser);
- label = do_case (loc1, exp1, NULL_TREE, std_attrs);
+ label = do_case (loc1, exp1o, expr.original_type ? expr.original_type : TREE_TYPE (exp1o), NULL_TREE, std_attrs);
}
else if (c_parser_next_token_is (parser, CPP_ELLIPSIS))
{
@@ -7834,7 +7840,7 @@ c_parser_label (c_parser *parser, tree std_attrs)
NULL),
true, true).value;
if (c_parser_require (parser, CPP_COLON, "expected %<:%>"))
- label = do_case (loc1, exp1, exp2, std_attrs);
+ label = do_case (loc1, exp1o, expr.original_type ? expr.original_type : TREE_TYPE (exp1o), exp2, std_attrs);
}
else
c_parser_error (parser, "expected %<:%> or %<...%>");
@@ -7843,7 +7849,7 @@ c_parser_label (c_parser *parser, tree std_attrs)
{
c_parser_consume_token (parser);
if (c_parser_require (parser, CPP_COLON, "expected %<:%>"))
- label = do_case (loc1, NULL_TREE, NULL_TREE, std_attrs);
+ label = do_case (loc1, NULL_TREE, NULL_TREE, NULL_TREE, std_attrs);
}
else
{
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 6e40f7edf02..75e92e6f48e 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -12777,7 +12777,53 @@ tree
do_case (location_t loc, tree low_value, tree high_value, tree attrs)
{
tree label = NULL_TREE;
+ tree original_low_value = low_value;
+ tree original_high_value = high_value;
+ if (low_value && TREE_CODE (low_value) != INTEGER_CST)
+ {
+ low_value = c_fully_fold (low_value, false, NULL);
+ if (TREE_CODE (low_value) == INTEGER_CST)
+ pedwarn (loc, OPT_Wpedantic,
+ "case label is not an integer constant expression");
+ }
+
+ if (high_value && TREE_CODE (high_value) != INTEGER_CST)
+ {
+ high_value = c_fully_fold (high_value, false, NULL);
+ if (TREE_CODE (high_value) == INTEGER_CST)
+ pedwarn (input_location, OPT_Wpedantic,
+ "case label is not an integer constant expression");
+ }
+ if (c_switch_stack == NULL)
+ {
+ if (low_value)
+ error_at (loc, "case label not within a switch statement");
+ else
+ error_at (loc, "%<default%> label not within a switch statement");
+ return NULL_TREE;
+ }
+
+ if (c_check_switch_jump_warnings (c_switch_stack->bindings,
+ EXPR_LOCATION (c_switch_stack->switch_stmt),
+ loc))
+ return NULL_TREE;
+
+ label = c_add_case_label (loc, c_switch_stack->cases,
+ SWITCH_STMT_COND (c_switch_stack->switch_stmt),
+ low_value, high_value,
+ original_low_value, original_high_value, attrs);
+ if (label == error_mark_node)
+ label = NULL_TREE;
+ return label;
+}
+
+tree
+do_case (location_t loc, tree low_value, tree low_original_type, tree high_value, tree attrs)
+{
+ tree label = NULL_TREE;
+ tree original_low_value = low_value;
+ tree original_high_value = high_value;
if (low_value && TREE_CODE (low_value) != INTEGER_CST)
{
low_value = c_fully_fold (low_value, false, NULL);
@@ -12810,7 +12856,8 @@ do_case (location_t loc, tree low_value, tree high_value, tree attrs)
label = c_add_case_label (loc, c_switch_stack->cases,
SWITCH_STMT_COND (c_switch_stack->switch_stmt),
- low_value, high_value, attrs);
+ low_value, high_value,
+ low_original_type, original_high_value, attrs);
if (label == error_mark_node)
label = NULL_TREE;
return label;
diff --git a/gcc/tree.cc b/gcc/tree.cc
index eab40008e8b..4ef82052971 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -2795,6 +2795,26 @@ build_case_label (tree low_value, tree high_value, tree label_decl)
CASE_LOW (t) = low_value;
CASE_HIGH (t) = high_value;
+ CASE_ORIGINAL_LOW (t) = NULL_TREE;
+ CASE_ORIGINAL_HIGH (t) = NULL_TREE;
+ CASE_LABEL (t) = label_decl;
+ CASE_CHAIN (t) = NULL_TREE;
+
+ return t;
+}
+
+tree
+build_case_label (tree low_value, tree high_value, tree original_low_value, tree original_high_value, tree label_decl)
+{
+ tree t = make_node (CASE_LABEL_EXPR);
+
+ TREE_TYPE (t) = void_type_node;
+ SET_EXPR_LOCATION (t, DECL_SOURCE_LOCATION (label_decl));
+
+ CASE_LOW (t) = low_value;
+ CASE_HIGH (t) = high_value;
+ CASE_ORIGINAL_LOW (t) = original_low_value;
+ CASE_ORIGINAL_HIGH (t) = original_high_value;
CASE_LABEL (t) = label_decl;
CASE_CHAIN (t) = NULL_TREE;
diff --git a/gcc/tree.def b/gcc/tree.def
index 44871367d0d..b6213d1fa39 100644
--- a/gcc/tree.def
+++ b/gcc/tree.def
@@ -1006,7 +1006,7 @@ DEFTREECODE (SWITCH_EXPR, "switch_expr", tcc_statement, 2)
Operand 3 is CASE_CHAIN. This operand is only used in tree-cfg.cc to
speed up the lookup of case labels which use a particular edge in
the control flow graph. */
-DEFTREECODE (CASE_LABEL_EXPR, "case_label_expr", tcc_statement, 4)
+DEFTREECODE (CASE_LABEL_EXPR, "case_label_expr", tcc_statement, 6)
/* Used to represent an inline assembly statement. ASM_STRING returns a
STRING_CST for the instruction (e.g., "mov x, y"). ASM_OUTPUTS,
diff --git a/gcc/tree.h b/gcc/tree.h
index aaaf703186c..c7a51036a8e 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -1407,6 +1407,8 @@ class auto_suppress_location_wrappers
#define CASE_HIGH(NODE) TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 1)
#define CASE_LABEL(NODE) TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 2)
#define CASE_CHAIN(NODE) TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 3)
+#define CASE_ORIGINAL_LOW(NODE) TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 4)
+#define CASE_ORIGINAL_HIGH(NODE) TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 5)
/* The operands of a TARGET_MEM_REF. Operands 0 and 1 have to match
corresponding MEM_REF operands. */
@@ -4743,6 +4745,8 @@ extern tree copy_list (tree);
extern tree build_case_label (tree, tree, tree);
+extern tree build_case_label (tree, tree, tree, tree, tree);
+
/* Make a BINFO. */
extern tree make_tree_binfo (unsigned CXX_MEM_STAT_INFO);
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.