Re: [PATCH 4/9] dfa: use a separate data type for grps

[Jim Meyering]

Paolo Bonzini wrote:
> * src/dfa.c (leaf_set): New.
> (dfastate): Use leaf_set for grps, as the constraint field is unused.
> ---
>  src/dfa.c |   25 +++++++++++++++++--------
>  1 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/dfa.c b/src/dfa.c
> index aa87f87..f50fdd0 100644
> --- a/src/dfa.c
> +++ b/src/dfa.c
> @@ -249,6 +249,13 @@ typedef struct
>    int nelem;                 /* Number of elements in this set. */
>  } position_set;
>
> +/* Sets of leaves are also stored as arrays. */
> +typedef struct
> +{
> +  unsigned int *elems;               /* Elements of this position set. */
> +  size_t nelem;                      /* Number of elements in this set. */
> +} leaf_set;

ACK.
However, please mention in the log that this decreases memory usage.  E.g.,

    dfa: use a more compact type for grps

    * src/dfa.c (leaf_set): New type.
    (dfastate): Use the smaller type, leaf_set, for grps.
    Its prior type contained an unused constraint field.

>  /* A state of the dfa consists of a set of positions, some flags,
>     and the token value of the lowest-numbered position of the state that
>     contains an END token. */
> @@ -2344,7 +2351,7 @@ dfaanalyze (struct dfa *d, int searchflag)
>  void
>  dfastate (int s, struct dfa *d, int trans[])
>  {
> -  position_set *grps;                /* As many as will ever be needed. */
> +  leaf_set *grps;            /* As many as will ever be needed. */
>    charclass *labels;         /* Labels corresponding to the groups. */
>    int ngrps = 0;             /* Number of groups actually used. */
>    position pos;                      /* Current position being considered. */
> @@ -2466,13 +2473,15 @@ dfastate (int s, struct dfa *d, int trans[])
>                copyset(leftovers, labels[ngrps]);
>                copyset(intersect, labels[j]);
>                MALLOC(grps[ngrps].elems, d->nleaves);
> -              copy(&grps[j], &grps[ngrps]);
> +              memcpy(grps[ngrps].elems, grps[j].elems,
> +                     grps[j].nelem * sizeof(unsigned int));
> +              grps[ngrps].nelem = grps[j].nelem;

While slightly longer, the following is also slightly safer.
If the type of "elems" ever changes, we won't need to realize the
"sizeof(unsigned int)" above must also be changed to match that new type:

                 memcpy(grps[ngrps].elems, grps[j].elems,
                        grps[j].nelem * sizeof(*(grps[j].elems)));