Re: arcfour

[Paul Eggert]

Some fairly minor comments:

Simon Josefsson <address@hidden> writes:

> +#define MOD256(n) ((n) & (ARCFOUR_SBOX_SIZE - 1))

That is a strange name to use.  I expected the definiens to be ((n) &
255).  Why not use the name MOD_ARCFOUR_SBOX_SIZE, or some shorter
variant perhaps?

If the code going to say MOD256 elsewhere we might as well just
open-code the &255 and dispense with the macro entirely.....

> +  register size_t i = context->idx_i;
> +  register size_t j = context->idx_j;

These days there's no need for 'register'.  Similarly for the
other occurrences of 'register'.  It just clutters up the code.

> +  register char *sbox = context->sbox;

For consistency, shouldn't arcfour_setkey also have this internal
variable?  (I doubt whether it'll affect the generated code, if
optimization is enabled; it's just a style thing.)

> +      i = MOD256(i + 1);
> +      j = MOD256(j + sbox[i]);

GNU style suggests a space before the paren, even for macro calls.
Similarly for the other occurrences of "MOD256(".

> +#define ARCFOUR_SBOX_SIZE 256
> +
> +typedef struct
> +{
> +  size_t idx_i, idx_j;
> +  char sbox[ARCFOUR_SBOX_SIZE];
> +} arcfour_context;

Why must these be in arcfour.h?  Shouldn't all this private to
arcfour.c?

You can replace the above lines with 'struct arcfour_context;' and
then move them into arcfour.c (replacing 'arcfour_context' with
'struct arcfour_context' everywhere).