Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> writes:
> On Thu, Jul 23, 2015 at 01:11:36AM +0200, Nicolai Stange wrote:
>> 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.
>
>> +/*
>> + * 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:
>> + 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.
Admittedly, this looks way better.
I'll change that to
void expr_set_flag(unsigned *flag, ...);
and likewise for the clearing guy.
>
> 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".
>
>> +/*
>> + * Remove any "Constant" [6.4.4] flag, but retain the "constant
>> + * expression" [6.6] flags.
>> + * Used to merge the constantness flags of primary subexpressions
>> + * 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)?
Not at all.
> Shouldn't this more directly implement the desciption of the function:
> "Remove any 'Constant' flag but retain ... ?
Yes. I will change this.
>> +/* 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.
> Yours,
> Luc
Again, thank you very much for your valuable review!
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
