Re: [bug-gnulib] Re: base64

[Paul Eggert]

Simon Josefsson <address@hidden> writes:

> base64.c:277: warning: comparison is always false due to limited range of 
> data type
>
> But I guess that is harmless.

Yes.  These days that warning is counterproductive more often than
not, in my experience.

> + * int ok = base64_decode_alloc (in, inlen, &out, &outlen);

int -> bool

> +/* Get UCHAR_MAX. */
> +#include <limits.h>

This shouldn't be needed any more.

> +                 + (--inlen ? (to_uchar (in[1]) >> 4) : 0)) & 0x3f];

A very minor point: the parentheses aren't needed around the then-part.
(This isn't the only instance.)

> +  size_t outlen = 1 + BASE64_LENGTH(inlen);

Missing space before '('.  Similarly for other occurrences of BASE64_LENGTH
(including comments).

> +static inline bool isb64 (char ch)
> +{
> +  if (to_uchar (ch) > 255)
> +    return false;
> +  if (b64[to_uchar (ch)] < 0)
> +    return false;
> +  return true;
> +}

You can simplify this a bit by making the argument to isb64 be of type
unsigned char rather than char.  That way, you can avoid the use of
to_uchar in this function.  Also, it might make it slightly clearer to
return a boolean expression.  And the name of the function should be
in column 1.  Something like this, perhaps:

   static inline bool
   isb64 (unsigned char ch)
   {
     return ch <= 255 && 0 <= b64[ch];
   }


> +           *out++ = ((b64[to_uchar (in[1])] << 4) & 0xf0)
> +             | (b64[to_uchar (in[2])] >> 2);

In expressions like this the usual GNU style is to add parentheses for
Emacs indenting, e.g.:

  *out++ = (((b64[to_uchar (in[1])] << 4) & 0xf0)
            | (b64[to_uchar (in[2])] >> 2));


> +  *outlen = *outlen - outleft;

Clearer would be:

   *outlen -= outleft;

> +  size_t needlen;
> +
> +  /* We want to compute 3 * inlen / 4, but to avoid overflow checking,
> +     we compute 3 * (inlen / 4) and re-add the truncated values.
> +
> +     FIXME? 3*inlen/4 may allocate one 1 or 2 bytes too much,
> +     depending on input (i.e., need 2 bytes less if input ends with
> +     '==' and 1 byte less if input end with '=').  The value returned
> +     in *OUTLEN will be correct, though. */
> +  needlen = 3 * (inlen / 4)
> +    + ((inlen % 4 == 2) ? 1 : 0)
> +    + ((inlen % 4 == 3) ? 2 : 0);

It'd be faster and simpler to do the following, which uses slightly
more memory, but the GNU tradition is to trade space for speed:

   /* This may allocate a few bytes too much, depending on input,
      but it's not worth the extra CPU time to compute the exact amount.
      The exact amount is 3 * inlen / 4, minus 1 if the input ends
      with "=" and minus another 1 if the input ends with "==".
      Dividing before multiplying avoids the possibility of overflow.  */
   size_t needlen = 3 * (inlen / 4) + 2;

> +  AC_REQUIRE([gl_SIZE_MAX])

This isn't needed any more.


Does performance matter in the implementation of these functions?  If
so, you might consider using "restrict" in the prototypes of functions
that accept two or more pointers to storage that might overlap, e.g.:

  void base64_encode (char const * restrict in, size_t inlen,
                      char * restrict out, size_t outlen);

and similarly for the other functions that shouldn't accept pointers
to overlapping storage.  This will improve the quality of the code
generated, with modern C compilers anyway, because they can use the
"restrict" information to avoid redundant loads.  C99 says two
pointers might point to overlapping storage if they point to
compatible types, or if one is char * or unsigned char *.