Re: [bug-gawk] useless array length abstraction

[Andrew J. Schorr]

On Sat, Jan 05, 2019 at 01:51:31PM -0500, Andrew J. Schorr wrote:
> Hmmm. I'm not convinced. The existing assoc_length macro involves an indirect
> function call that accomplishes nothing. It's a waste of cycles. And the code
> is not consistent about using the assoc_length macro. For example, in
> gawkapi.c, there are a bunch of places where the table_size field is accessed
> directly instead of using the assoc_length macro.
> 
> I think the smart move is to remove the alength entry in array_funcs
> and get rid of all of this cruft. Why have a useless abstraction in the
> code that serves no purpose?

I propose the attached patch that removes the alength function and cleans
up this issue.

But taking another step back, I'm not sure why these array methods are
implemented as an array of function pointers instead of as a struct
with a field for each method. In other words, why do we have:

typedef struct exp_node **(*afunc_t)(struct exp_node *, struct exp_node *);
...
typedef struct exp_node {
...
                                afunc_t *lp;
...
} NODE;

#define ainit_ind       0
#define ainit           array_funcs[ainit_ind]
#define atypeof_ind     1
#define atypeof         array_funcs[atypeof_ind]
#define alookup_ind     2
#define alookup         array_funcs[alookup_ind]
#define aexists_ind     3
#define aexists         array_funcs[aexists_ind]
#define aclear_ind      4
#define aclear          array_funcs[aclear_ind]
#define aremove_ind     5
#define aremove         array_funcs[aremove_ind]
#define alist_ind       6
#define alist           array_funcs[alist_ind]
#define acopy_ind       7
#define acopy           array_funcs[acopy_ind]
#define adump_ind       8
#define adump           array_funcs[adump_ind]
#define astore_ind      9
#define astore          array_funcs[astore_ind]
#define NUM_AFUNCS      10              /* # of entries in array_funcs */

Instead of more clearly:

typedef struct {
        const char *name;       /* str or int or cint */
        afunc_t init;
        afunc_t typeof;
        afunc_t lookup;
        afunc_t exists;
        afunc_t clear;
        afunc_t remove;
        afunc_t list;
        afunc_t copy;
        afunc_t dump;
        afunc_t store;
} array_funcs_t;

#define ainit           array_funcs->init
#define atypeof         array_funcs->typeof
#define alookup         array_funcs->lookup
#define aexists         array_funcs->exists
#define aclear          array_funcs->aclear
#define aremove         array_funcs->remove
#define alist           array_funcs->list
#define acopy           array_funcs->copy
#define adump           array_funcs->dump
#define astore          array_funcs->store

And then change the type of "lp" in NODE from 'afunc_t *'
to 'array_funcs_t *'.

This would also allow having different function signatures
for the different methods, instead of the straitjacket that
we put ourselves into by using an array. For example,
the typeof method should arguably return a boolean value,
and it doesn't really need the first argument.

Regards,
Andy

array.patch
Description: Text document