expression: introduce additional expression constness tracking flags

RFC PATCH Superseded Nicolai Stange 3 participants 5 messages Jul 22, 2015 – Jan 11, 2016 Sparse checker

lore

Share & tools

lore

Nicolai Stange

author

2015-07-22 23:11 UTC #0 lore raw

RFC PATCH 1/13 expression: introduce additional expression constness tracking flags

Prepare for a more fine-grained tracking of expression constness in the sense of C99 [6.4.4, 6.6].

User-visible behaviour remains unchanged.

The current implementation tags an expression with either combination of the flags Int_const_expr and Float_literal, the latter being only used to tell that
(int).0
is indeed an integer constant expression.

Even if sparse attempted to verify that initializers for static storage duration objects are constant expressions [6.7.8(4)] (which it currently does not), it could not tell reliably.
Examples:
1.)
static float a = { (float)0 }; /* allowed by C99 */ '(float)0' is not an integer constant expression, but an arithmetic one.
2.)
enum { b = 0 };
static void *c = { (void*)b }; /* disallowed by C99 */ References to enum members are not allowed in address constants [6.6(9)] and thus, the initializer is not a constant expression at all.

Introduce a broader set of constness tracking flags, resembling the four types of constants [6.4.4] (integer, floating, enumeration, character) and the three types of constant expressions [6.6] (integer, arithmetic, address). Use helper functions to consistently set and clear these flags as they are not completely independent.

Signed-off-by Nicolai Stange <[email protected]>

---
 evaluate.c   |  69 ++++++++++++++++++-------------
 expand.c     |   2 +-
 expression.c |  66 ++++++++++++++++++-----------
 expression.h | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 212 insertions(+), 58 deletions(-)

evaluate.c +41 ~~-28~~

index 035e448..7324fb4 100644
@@ -400,7 +400,7 @@ static struct symbol *bad_expr_type(struct expression *expr)
 		break;
 	}
 
-	expr->flags = 0;
+	expr->flags = EXPR_FLAG_NONE;
 	return expr->ctype = &bad_ctype;
 }

@@ -880,8 +880,9 @@ static struct symbol *evaluate_logical(struct expression *expr)
 	/* the result is int [6.5.13(3), 6.5.14(3)] */
 	expr->ctype = &int_ctype;
 	if (expr->flags) {
-		if (!(expr->left->flags & expr->right->flags & Int_const_expr))
-			expr->flags = 0;
+		if (!(expr->left->flags & expr->right->flags &
+				EXPR_FLAG_INT_CONST_EXPR))
+			expr->flags = EXPR_FLAG_NONE;
 	}
 	return &int_ctype;
 }

@@ -894,8 +895,9 @@ static struct symbol *evaluate_binop(struct expression *expr)
 	int op = expr->op;
 
 	if (expr->flags) {
-		if (!(expr->left->flags & expr->right->flags & Int_const_expr))
-			expr->flags = 0;
+		if (!(expr->left->flags & expr->right->flags &
+				EXPR_FLAG_INT_CONST_EXPR))
+			expr->flags = EXPR_FLAG_NONE;
 	}
 
 	/* number op number */

@@ -986,7 +988,7 @@ static inline int is_null_pointer_constant(struct expression *e)
 {
 	if (e->ctype == &null_ctype)
 		return 1;
-	if (!(e->flags & Int_const_expr))
+	if (!(e->flags & EXPR_FLAG_INT_CONST_EXPR))
 		return 0;
 	return is_zero_constant(e) ? 2 : 0;
 }

@@ -1001,8 +1003,9 @@ static struct symbol *evaluate_compare(struct expression *expr)
 	const char *typediff;
 
 	if (expr->flags) {
-		if (!(expr->left->flags & expr->right->flags & Int_const_expr))
-			expr->flags = 0;
+		if (!(expr->left->flags & expr->right->flags &
+				EXPR_FLAG_INT_CONST_EXPR))
+			expr->flags = EXPR_FLAG_NONE;
 	}
 
 	/* Type types? */

@@ -1120,10 +1123,11 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
 	}
 
 	if (expr->flags) {
-		int flags = expr->conditional->flags & Int_const_expr;
+		int flags = (expr->conditional->flags &
+			expr_set_flag_mask(EXPR_FLAG_INT_CONST_EXPR));
 		flags &= (*true)->flags & expr->cond_false->flags;
 		if (!flags)
-			expr->flags = 0;
+			expr->flags = EXPR_FLAG_NONE;
 	}
 
 	lclass = classify_type(ltype, &ltype);

@@ -1681,7 +1685,7 @@ static struct symbol *evaluate_addressof(struct expression *expr)
 	}
 	ctype = op->ctype;
 	*expr = *op->unop;
-	expr->flags = 0;
+	expr->flags = EXPR_FLAG_NONE;
 
 	if (expr->type == EXPR_SYMBOL) {
 		struct symbol *sym = expr->symbol;

@@ -1709,7 +1713,7 @@ static struct symbol *evaluate_dereference(struct expression *expr)
 	/* Simplify: *&(expr) => (expr) */
 	if (op->type == EXPR_PREOP && op->op == '&') {
 		*expr = *op->unop;
-		expr->flags = 0;
+		expr->flags = EXPR_FLAG_NONE;
 		return expr->ctype;
 	}

@@ -1799,8 +1803,8 @@ static struct symbol *evaluate_sign(struct expression *expr)
 {
 	struct symbol *ctype = expr->unop->ctype;
 	int class = classify_type(ctype, &ctype);
-	if (expr->flags && !(expr->unop->flags & Int_const_expr))
-		expr->flags = 0;
+	if (expr->flags && !(expr->unop->flags & EXPR_FLAG_INT_CONST_EXPR))
+		expr->flags = EXPR_FLAG_NONE;
 	/* should be an arithmetic type */
 	if (!(class & TYPE_NUM))
 		return bad_expr_type(expr);

@@ -1854,8 +1858,9 @@ static struct symbol *evaluate_preop(struct expression *expr)
 		return evaluate_postop(expr);
 
 	case '!':
-		if (expr->flags && !(expr->unop->flags & Int_const_expr))
-			expr->flags = 0;
+		if (expr->flags && !(expr->unop->flags &
+					EXPR_FLAG_INT_CONST_EXPR))
+			expr->flags = EXPR_FLAG_NONE;
 		if (is_safe_type(ctype))
 			warning(expr->pos, "testing a 'safe expression'");
 		if (is_float_type(ctype)) {

@@ -2734,14 +2739,19 @@ static struct symbol *evaluate_cast(struct expression *expr)
 
 	class1 = classify_type(ctype, &t1);
 
-	/* cast to non-integer type -> not an integer constant expression */
-	if (!is_int(class1))
-		expr->flags = 0;
+	/* cast to non-numeric type -> not an arithmetic expression */
+	if (!(class1 & TYPE_NUM))
+		expr->flags &=
+			~expr_clear_flag_mask(EXPR_FLAG_ARITH_CONST_EXPR);
+	/* cast to float type -> not an integer constant expression */
+	else if (class1 & TYPE_FLOAT)
+		expr->flags &= ~expr_clear_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
 	/* if argument turns out to be not an integer constant expression *and*
 	   it was not a floating literal to start with -> too bad */
-	else if (expr->flags == Int_const_expr &&
-		!(target->flags & Int_const_expr))
-		expr->flags = 0;
+	else if (!(target->flags &
+			(EXPR_FLAG_INT_CONST_EXPR | EXPR_FLAG_FP_CONST)))
+		expr->flags &= ~expr_clear_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
+
 	/*
 	 * You can always throw a value away by casting to
 	 * "void" - that's an implicit "force". Note that

@@ -2803,7 +2813,7 @@ static struct symbol *evaluate_cast(struct expression *expr)
 			"cast adds address space to expression (<asn:%d>)", as1);
 
 	if (!(t1->ctype.modifiers & MOD_PTRINHERIT) && class1 == TYPE_PTR &&
-	    !as1 && (target->flags & Int_const_expr)) {
+	    !as1 && (target->flags & EXPR_FLAG_INT_CONST_EXPR)) {
 		if (t1->ctype.base_type == &void_ctype) {
 			if (is_zero_constant(target)) {
 				/* NULL */

@@ -2933,7 +2943,7 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 		}
 		ctype = field;
 		expr->type = EXPR_VALUE;
-		expr->flags = Int_const_expr;
+		expr->flags = expr_set_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
 		expr->value = offset;
 		expr->taint = 0;
 		expr->ctype = size_t_ctype;

@@ -2951,7 +2961,8 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 		ctype = ctype->ctype.base_type;
 		if (!expr->index) {
 			expr->type = EXPR_VALUE;
-			expr->flags = Int_const_expr;
+			expr->flags
+				= expr_set_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
 			expr->value = 0;
 			expr->taint = 0;
 			expr->ctype = size_t_ctype;

@@ -2968,13 +2979,14 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 			m = alloc_const_expression(expr->pos,
 						   bits_to_bytes(ctype->bit_size));
 			m->ctype = size_t_ctype;
-			m->flags = Int_const_expr;
+			m->flags = expr_set_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
 			expr->type = EXPR_BINOP;
 			expr->left = idx;
 			expr->right = m;
 			expr->op = '*';
 			expr->ctype = size_t_ctype;
-			expr->flags = m->flags & idx->flags & Int_const_expr;
+			expr->flags = m->flags & idx->flags &
+				expr_set_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
 		}
 	}
 	if (e) {

@@ -2985,7 +2997,8 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
 		if (!evaluate_expression(e))
 			return NULL;
 		expr->type = EXPR_BINOP;
-		expr->flags = e->flags & copy->flags & Int_const_expr;
+		expr->flags = e->flags & copy->flags &
+			expr_set_flag_mask(EXPR_FLAG_INT_CONST_EXPR);
 		expr->op = '+';
 		expr->ctype = size_t_ctype;
 		expr->left = copy;

expand.c +1 -1

index 0f6720c..afca611 100644
@@ -1212,7 +1212,7 @@ static int expand_statement(struct statement *stmt)
 
 static inline int bad_integer_constant_expression(struct expression *expr)
 {
-	if (!(expr->flags & Int_const_expr))
+	if (!(expr->flags & EXPR_FLAG_INT_CONST_EXPR))
 		return 1;
 	if (expr->taint & Taint_comma)
 		return 1;

expression.c +42 ~~-24~~

163 lines not on this page · raw message

expression.h +128 -5

145 lines not on this page · raw message

Signature · 1 line

2.4.5

Sam Ravnborg

2015-08-01 13:07 UTC #1 lore raw

Quoted 4 lines · Nicolai Stange

Prepare for a more fine-grained tracking of expression constness in the sense of C99 [6.4.4, 6.6].
User-visible behaviour remains unchanged.

Do this imply that you cannot write any testcase that fails before but is fixed with this serie?

Sam

Luc Van Oostenryck

2016-01-09 17:03 UTC #2 lore raw

In reply to Nicolai Stange

Quoted 3 lines · Nicolai Stange

Prepare for a more fine-grained tracking of expression constness in the sense of C99 [6.4.4, 6.6].

I have a few remarks/questions/suggestions here under.

Quoted 92 lines

expression.h +92 -0

+/*
+ * Flags for tracking the promotion of various attributes from
+ * subexpressions to their parents.
+ *
+ * Currently, they only cope with an expression's constness as defined
+ * by C99.
+ *
+ * The flags are not independent as one might imply another. Use
+ * expr_set_flag_mask() and expr_clear_flag_mask() for setting and
+ * clearing a particular flag.
+ */
+enum expression_flags {
+	EXPR_FLAG_NONE = 0,
+	/*
+	 * A constant in the sense of [6.4.4]:
+	 * - Integer constant [6.4.4.1]
+	 * - Floating point constant [6.4.4.2]
+	 * - Enumeration constant [6.4.4.3]
+	 * - Character constant [6.4.4.4]
+	 */
+	EXPR_FLAG_INT_CONST = (1 << 0),
+	EXPR_FLAG_FP_CONST = (1 << 1),
+	EXPR_FLAG_ENUM_CONST = (1 << 2),
+	EXPR_FLAG_CHAR_CONST = (1 << 3),
+
+	/*
+	 * A constant expression in the sense of [6.6]:
+	 * - integer constant expression [6.6(6)]
+	 * - arithmetic constant expression [6.6(8)]
+	 * - address constanr [6.6(9)]
+	 */
+	EXPR_FLAG_INT_CONST_EXPR = (1 << 4),
+	EXPR_FLAG_ARITH_CONST_EXPR = (1 << 5),
+	EXPR_FLAG_ADDR_CONST_EXPR = (1 << 6),
+};
+
+/*
+ * Calculate a mask to be or'ed in in order to set a particular
+ * expression flag.
+ *
+ * Only one single flag from enum expression_flags is allowed at a
+ * time.
+ */
+static inline enum expression_flags expr_set_flag_mask
+	(const enum expression_flags flag)
+{
+	/* obey the implications */
+	enum expression_flags implied_flags = EXPR_FLAG_NONE;
+
+	switch (flag) {
+	case EXPR_FLAG_INT_CONST:
+	case EXPR_FLAG_ENUM_CONST:
+	case EXPR_FLAG_CHAR_CONST:
+		implied_flags |= EXPR_FLAG_INT_CONST_EXPR;
+	/* fallthrough */
+	case EXPR_FLAG_FP_CONST:
+	case EXPR_FLAG_INT_CONST_EXPR:
+		implied_flags |= EXPR_FLAG_ARITH_CONST_EXPR;
+	/* fallthrough */
+	case EXPR_FLAG_ARITH_CONST_EXPR:
+	case EXPR_FLAG_ADDR_CONST_EXPR:
+	case EXPR_FLAG_NONE:
+		break;
+	}
+
+	return (implied_flags | flag);
+}
+
+/*
+ * Calculate a mask to be negated and and'ed in in order to clear a
+ * particular expression flag.
+ *
+ * Only one single flag from enum expression_flags is allowed at a
+ * time.
+ */
+static inline enum expression_flags expr_clear_flag_mask
+	(const enum expression_flags flag)
+{
+	/* obey the implications */
+	enum expression_flags implied_flags = EXPR_FLAG_NONE;
+
+	switch (flag) {
+	case EXPR_FLAG_ARITH_CONST_EXPR:
+		implied_flags |= EXPR_FLAG_INT_CONST_EXPR;
+		implied_flags |= EXPR_FLAG_FP_CONST;
+	/* fallthrough */
+	case EXPR_FLAG_INT_CONST_EXPR:
+		implied_flags |= EXPR_FLAG_INT_CONST;
+		implied_flags |= EXPR_FLAG_ENUM_CONST;
+		implied_flags |= EXPR_FLAG_CHAR_CONST;
+	/* fallthrough */
+	case EXPR_FLAG_ADDR_CONST_EXPR:

expression.h enum expression_type { · line 171

Quoted 10 lines

expression.h +10 -0

+	case EXPR_FLAG_INT_CONST:
+	case EXPR_FLAG_FP_CONST:
+	case EXPR_FLAG_ENUM_CONST:
+	case EXPR_FLAG_CHAR_CONST:
+	case EXPR_FLAG_NONE:
+		break;
+	}
+
+	return (implied_flags | flag);
+}

Shouldn't the following be more explicit?

	flag = expr_set_flag_mask(0, ...);
	flag = expr_set_flag_mask(in_flag, ...);
	flag = expr_clear_flag_mask(in_flag, ...);

Yes, I know, it would need to duplicate the expr->flags at almost all calls.

Couldn't we get rid of those two function by separating the exclusive "bits" from the "sets"?
Something like:

	#define	__EXPR_FLAG_INT_CONST	(1 << 0)
	#define	__EXPR_FLAG_FP_CONST	(1 << 1)
	...
	#define	EXPR_FLAG_INT_CONST	(__EXPR_FLAG_INT_CONST |
					 __EXPR_FLAG_INT_CONST_EXPR |
					 __EXPR_FLAG_ARITH_CONST)

Quoted 4 lines

expression.h +4 -0

+/*
+ *  Remove any "Constant" [6.4.4] flag, but retain the "constant
+ * expression" [6.6] flags.
+ * Used to merge the constantness flags of primary subexpressions

expression.h enum expression_type { · line 186

Quoted 10 lines

expression.h +10 -0

+ * into their parent expressions' ones.
+ */
+static inline enum expression_flags expr_flags_decay_consts
+	(enum expression_flags flags)
+{
+	return (flags & ~(expr_clear_flag_mask(EXPR_FLAG_INT_CONST)
+			  | expr_clear_flag_mask(EXPR_FLAG_FP_CONST)
+			  | expr_clear_flag_mask(EXPR_FLAG_ENUM_CONST)
+			  | expr_clear_flag_mask(EXPR_FLAG_CHAR_CONST)));
+}

How is that different from:

	return flags & ~(EXPR_FLAG_INT_CONST
			|EXPR_FLAG_FP_CONST
			|EXPR_FLAG_ENUM_CONST
			|EXPR_FLAG_CHAR_CONST)?

Shouldn't this more directly implement the desciption of the function:
"Remove any 'Constant' flag but retain ... ?

expression.h enum expression_type { · line 195

Quoted 8 lines

expression.h +8 -0

+/* Purge any constantness related flag. */
+static inline enum expression_flags expr_flags_remove_consts
+	(enum expression_flags flags)
+{
+	return (flags &
+		~(expr_clear_flag_mask(EXPR_FLAG_ARITH_CONST_EXPR)
+		  | expr_clear_flag_mask(EXPR_FLAG_ADDR_CONST_EXPR)));
+}

Same as above with the appropriate changes.

Yours,
Luc

Nicolai Stange

author

2016-01-09 22:20 UTC #3 lore raw

Quoted 111 lines · Luc Van Oostenryck

Quoted 3 lines · Nicolai Stange

Prepare for a more fine-grained tracking of expression constness in the sense of C99 [6.4.4, 6.6].

I have a few remarks/questions/suggestions here under.

Quoted 92 lines

expression.h +92 -0

+/*
+ * Flags for tracking the promotion of various attributes from
+ * subexpressions to their parents.
+ *
+ * Currently, they only cope with an expression's constness as defined
+ * by C99.
+ *
+ * The flags are not independent as one might imply another. Use
+ * expr_set_flag_mask() and expr_clear_flag_mask() for setting and
+ * clearing a particular flag.
+ */
+enum expression_flags {
+	EXPR_FLAG_NONE = 0,
+	/*
+	 * A constant in the sense of [6.4.4]:
+	 * - Integer constant [6.4.4.1]
+	 * - Floating point constant [6.4.4.2]
+	 * - Enumeration constant [6.4.4.3]
+	 * - Character constant [6.4.4.4]
+	 */
+	EXPR_FLAG_INT_CONST = (1 << 0),
+	EXPR_FLAG_FP_CONST = (1 << 1),
+	EXPR_FLAG_ENUM_CONST = (1 << 2),
+	EXPR_FLAG_CHAR_CONST = (1 << 3),
+
+	/*
+	 * A constant expression in the sense of [6.6]:
+	 * - integer constant expression [6.6(6)]
+	 * - arithmetic constant expression [6.6(8)]
+	 * - address constanr [6.6(9)]
+	 */
+	EXPR_FLAG_INT_CONST_EXPR = (1 << 4),
+	EXPR_FLAG_ARITH_CONST_EXPR = (1 << 5),
+	EXPR_FLAG_ADDR_CONST_EXPR = (1 << 6),
+};
+
+/*
+ * Calculate a mask to be or'ed in in order to set a particular
+ * expression flag.
+ *
+ * Only one single flag from enum expression_flags is allowed at a
+ * time.
+ */
+static inline enum expression_flags expr_set_flag_mask
+	(const enum expression_flags flag)
+{
+	/* obey the implications */
+	enum expression_flags implied_flags = EXPR_FLAG_NONE;
+
+	switch (flag) {
+	case EXPR_FLAG_INT_CONST:
+	case EXPR_FLAG_ENUM_CONST:
+	case EXPR_FLAG_CHAR_CONST:
+		implied_flags |= EXPR_FLAG_INT_CONST_EXPR;
+	/* fallthrough */
+	case EXPR_FLAG_FP_CONST:
+	case EXPR_FLAG_INT_CONST_EXPR:
+		implied_flags |= EXPR_FLAG_ARITH_CONST_EXPR;
+	/* fallthrough */
+	case EXPR_FLAG_ARITH_CONST_EXPR:
+	case EXPR_FLAG_ADDR_CONST_EXPR:
+	case EXPR_FLAG_NONE:
+		break;
+	}
+
+	return (implied_flags | flag);
+}
+
+/*
+ * Calculate a mask to be negated and and'ed in in order to clear a
+ * particular expression flag.
+ *
+ * Only one single flag from enum expression_flags is allowed at a
+ * time.
+ */
+static inline enum expression_flags expr_clear_flag_mask
+	(const enum expression_flags flag)
+{
+	/* obey the implications */
+	enum expression_flags implied_flags = EXPR_FLAG_NONE;
+
+	switch (flag) {
+	case EXPR_FLAG_ARITH_CONST_EXPR:
+		implied_flags |= EXPR_FLAG_INT_CONST_EXPR;
+		implied_flags |= EXPR_FLAG_FP_CONST;
+	/* fallthrough */
+	case EXPR_FLAG_INT_CONST_EXPR:
+		implied_flags |= EXPR_FLAG_INT_CONST;
+		implied_flags |= EXPR_FLAG_ENUM_CONST;
+		implied_flags |= EXPR_FLAG_CHAR_CONST;
+	/* fallthrough */
+	case EXPR_FLAG_ADDR_CONST_EXPR:

expression.h enum expression_type { · line 171

Quoted 10 lines

expression.h +10 -0

+	case EXPR_FLAG_INT_CONST:
+	case EXPR_FLAG_FP_CONST:
+	case EXPR_FLAG_ENUM_CONST:
+	case EXPR_FLAG_CHAR_CONST:
+	case EXPR_FLAG_NONE:
+		break;
+	}
+
+	return (implied_flags | flag);
+}

Shouldn't the following be more explicit?

Quoted 4 lines

	flag = expr_set_flag_mask(0, ...);
	flag = expr_set_flag_mask(in_flag, ...);
	flag = expr_clear_flag_mask(in_flag, ...);
Yes, I know, it would need to duplicate the expr->flags at almost all calls.

Admittedly, this looks way better.

I'll change that to
void expr_set_flag(unsigned *flag, ...);
and likewise for the clearing guy.

Quoted 10 lines

Couldn't we get rid of those two function by separating the exclusive "bits" from the "sets"?
Something like:
	#define	__EXPR_FLAG_INT_CONST	(1 << 0)
	#define	__EXPR_FLAG_FP_CONST	(1 << 1)
	...
	#define	EXPR_FLAG_INT_CONST	(__EXPR_FLAG_INT_CONST |
					 __EXPR_FLAG_INT_CONST_EXPR |
					 __EXPR_FLAG_ARITH_CONST)

No, this won't work since the "implied" bit masks are in general different for setting and clearing a flag.

For example, "integer constant" (i.e. integer literal) implies "integer constant expression", but "not a integer constant" does not imply "not a integer constant expression".

Quoted 17 lines

Quoted 5 lines

expression.h +4 -0

+/*
+ *  Remove any "Constant" [6.4.4] flag, but retain the "constant
+ * expression" [6.6] flags.
+ * Used to merge the constantness flags of primary subexpressions

expression.h enum expression_type { · line 186

Quoted 10 lines

expression.h +10 -0

+ * into their parent expressions' ones.
+ */
+static inline enum expression_flags expr_flags_decay_consts
+	(enum expression_flags flags)
+{
+	return (flags & ~(expr_clear_flag_mask(EXPR_FLAG_INT_CONST)
+			  | expr_clear_flag_mask(EXPR_FLAG_FP_CONST)
+			  | expr_clear_flag_mask(EXPR_FLAG_ENUM_CONST)
+			  | expr_clear_flag_mask(EXPR_FLAG_CHAR_CONST)));
+}

How is that different from:

Quoted 4 lines

	return flags & ~(EXPR_FLAG_INT_CONST
			|EXPR_FLAG_FP_CONST
			|EXPR_FLAG_ENUM_CONST
			|EXPR_FLAG_CHAR_CONST)?

Not at all.

Quoted 2 lines

Shouldn't this more directly implement the desciption of the function:
"Remove any 'Constant' flag but retain ... ?

Yes. I will change this.

Quoted 11 lines

expression.h enum expression_type { · line 195

Quoted 8 lines

expression.h +8 -0

+/* Purge any constantness related flag. */
+static inline enum expression_flags expr_flags_remove_consts
+	(enum expression_flags flags)
+{
+	return (flags &
+		~(expr_clear_flag_mask(EXPR_FLAG_ARITH_CONST_EXPR)
+		  | expr_clear_flag_mask(EXPR_FLAG_ADDR_CONST_EXPR)));
+}

Same as above with the appropriate changes.

Ditto.

Quoted 2 lines

Yours,
Luc

Again, thank you very much for your valuable review!

Luc Van Oostenryck

2016-01-11 17:54 UTC #4 lore raw

Quoted 25 lines · Nicolai Stange

Quoted 6 lines · Luc Van Oostenryck
Shouldn't the following be more explicit?
	flag = expr_set_flag_mask(0, ...);
	flag = expr_set_flag_mask(in_flag, ...);
	flag = expr_clear_flag_mask(in_flag, ...);
Yes, I know, it would need to duplicate the expr->flags at almost all calls.
Admittedly, this looks way better.
I'll change that to
void expr_set_flag(unsigned *flag, ...);
and likewise for the clearing guy.
Quoted 10 lines
Couldn't we get rid of those two function by separating the exclusive "bits" from the "sets"?
Something like:
	#define	__EXPR_FLAG_INT_CONST	(1 << 0)
	#define	__EXPR_FLAG_FP_CONST	(1 << 1)
	...
	#define	EXPR_FLAG_INT_CONST	(__EXPR_FLAG_INT_CONST |
					 __EXPR_FLAG_INT_CONST_EXPR |
					 __EXPR_FLAG_ARITH_CONST)

Quoted 6 lines

No, this won't work since the "implied" bit masks are in general different for setting and clearing a flag.
For example, "integer constant" (i.e. integer literal) implies "integer constant expression", but "not a integer constant" does not imply "not a integer constant expression".

Yes, sure, but it could work with one set of such macro to add flags and another one to clear them.
I think it would be more clear and would avoid the need to have the two helper above.
Not that it is critical, though.

Yours,
Luc