Hi!
On Thu, Jun 17, 2021 at 10:18:55AM -0500, Bill Schmidt wrote:
> +enum bif_stanza
> +{
...
> + NUMBIFSTANZAS
> +};
Doing "NUM" things like this makes it valid to assign the NUM value to
a variable of this enum type, and importantly, the compiler cannot
warn for it then. So this is a bit of an antipattern.
> +/* Attributes of a builtin function. */
> +struct attrinfo
> +{
> + char isinit;
> + char isset;
[snip]
Is it nicer to have "bool" for these?
> +static int *bif_order;
This one probably can use a comment.
> +static bif_stanza
> +stanza_name_to_stanza (const char *stanza_name)
> +{
> + for (int i = 0; i < NUMBIFSTANZAS; i++)
> + if (!strcmp (stanza_name, stanza_map[i].stanza_name))
> + return stanza_map[i].stanza;
> + assert (false);
> +}
assert() compiles to nothing if NDEBUG is defined at the point you
include the header. That shouldn't happen, but please just make a
fatal() or similar function for this?
> + /* Append a number representing the order in which this function
> + was encountered to its name, and save in another lookup
> + structure. */
> + int orig_len = strlen (bifs[curr_bif].idname);
> + char *buf = (char *) malloc (orig_len + 7);
> + strcpy (buf, bifs[curr_bif].idname);
> + buf[orig_len] = ':';
> + char numstr[6];
> + sprintf (numstr, "%05d", curr_bif);
> + strcpy (&buf[orig_len + 1], numstr);
char *buf;
asprintf (&buf, "%s:%05d", bifs[curr_bif].idname, curr_bif);
> +#ifdef DEBUG
> + (*diag) ("pattern name is '%s'.\n", bifs[curr_bif].patname);
> +#endif
Maybe you can do the DEBUG thing inside the diag function itself, so
that you do not need the macro check at every use? Or use a different
function, even. (That has nothing to do with this patch in particular
of course).
Okay for trunk. Thanks!
Segher
