On Wed, Sep 25, 2019 at 11:00:12AM +0100, Ben Dooks wrote:
> diff --git a/parse.c b/parse.c
> index f291e24..583a82c 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -87,7 +87,7 @@ static attr_t
> attribute_address_space, attribute_context,
> attribute_designated_init,
> attribute_transparent_union, ignore_attribute,
> - attribute_mode, attribute_force;
> + attribute_mode, attribute_force, attribute_format;
I prefer that you simply insert for the attribute without touvhing the
others one:
+ invalid_printf_format_args,
> typedef struct symbol *to_mode_t(struct symbol *);
>
> @@ -136,6 +136,11 @@ static void asm_modifier_inline(struct token *token, unsigned long *mods)
> asm_modifier(token, mods, MOD_INLINE);
> }
>
> +/* the types of printf style formatting from __attribute__((format)) */
> +enum {
> + FmtPrintf = 0, FmtScanf,
> +};
Please change this to:
FORMAT_PRINTF,
FORMAT_SCANF,
> @@ -1172,6 +1195,59 @@ static struct token *attribute_address_space(struct token *token, struct symbol
> return token;
> }
>
> +static int invalid_printf_format_args(long long start, long long at)
> +{
> + return start < 0 || at < 0 || (start == at && start > 0) ||
> + (start == 0 && at == 0);
> +}
The name suggest it is only used for printf format but the code below
uses it for all formats, please rename it.
I would prefer to have the reverse logics, check if the format is valid
and to have a simpler check, something like:
static bool validate_attribute_format(...)
{
return (start > 1) && ((at > start) || at == 0);
}
but since more validations are done after, I think it's best to simply
not use this helper and directly doing the checks and emitting the
approriate warning messages when needed ("index smaller than 1", ...).
> +static struct token *attribute_format(struct token *token, struct symbol *attr, struct decl_state *ctx)
> +{
...
> + ctx->ctype.printf_va_start = start;
> + ctx->ctype.printf_msg = at;
GCC's manpage call them 'string-index' & 'first-to-check'. Best to keep things
coherent and use the same names everywhere, for example 'index' & first' ?
> diff --git a/symbol.h b/symbol.h
> index ac43b31..7bb6f29 100644
> --- a/symbol.h
> +++ b/symbol.h
> @@ -103,6 +104,7 @@ struct ctype {
> struct context_list *contexts;
> struct ident *as;
> struct symbol *base_type;
> + unsigned short printf_va_start, printf_msg;
What about something like:
+ struct {
+ unsigned short index;
+ unsigned short first;
+ } format;
Also the validation should check that these are not bigger than
USHORT_MAX.
-- Luc
