Re: rewritten inttypes module

[Paul Eggert]

Thanks very much for tackling this!  It will
clean up quite a bit of code.  Some comments:

Bruno Haible <address@hidden> writes:

> + #if !defined PRId8 || PRI_MACROS_BROKEN
> + # undef PRId8
> + # define PRId8 "d"
> + #endif

C99 says macro definitions like this should be skipped if C++, unless
__STDC_FORMAT_MACROS is defined.  Also, the macro should be defined
only if the corresponding type exists (this is relevant only on weird,
perhaps nonexistent, hosts that have stdint.h but not inttypes.h, but
we might as well do things right).  Perhaps something like this
instead?

#if !defined PRId8 || PRI_MACROS_BROKEN
# undef PRId8
# if !defined __cplusplus || defined __STDC_FORMAT_MACROS
#  ifdef INT8_MAX
#   define PRId8 "d"
#  endif
# endif
#endif

Similarly for the other optional types, e.g., intptr_t, int64_t.

> + #if !defined PRIdFAST8 || PRI_MACROS_BROKEN
> + # undef PRIdFAST8
> + # if INT_FAST8_MAX > INT32_MAX
> + #  define PRIdFAST8 PRId64
> + # else
> + #  define PRIdFAST8 "d"
> + # endif
> + #endif

Shouldn't that be "INT_FAST8_MAX > INT_MAX"?  Similarly for the other
macros.  Something like this might be a bit more robust:

#if !defined PRIdFAST8 || PRI_MACROS_BROKEN
# undef PRIdFAST8
# if !defined __cplusplus || defined __STDC_FORMAT_MACROS
#  if INT_FAST8_MAX <= INT_MAX
#   define PRIdFAST8 "d"
#  elif INT_FAST8_MAX <= LONG_MAX
#   define PRIdFAST8 "ld"
#  else
#   define PRIdFAST8 PRId64
#  endif
# endif
#endif


> + /* Don't bother defining or declaring wcstoimax and wcstoumax, since
> +    wide-character functions like this are hardly useful.  */

hardly useful -> hardly ever useful

> + #include <stddef.h>
> + #define __STDC_LIMIT_MACROS 1 /* to make it work also in C++ mode */

Need to define __STDC_FORMAT_MACROS here too.

> + #include ABSOLUTE_INTTYPES_H

> + /* Same tests as in stdint.m4.  */

Shouldn't these common tests be factored out?  Or perhaps it's be
simpler and faster to look at gl_cv_header_working_stdint_h; if it is
not 'yes' then you can assume that inttypes.h is broken too.  That's a
fairly safe assumption, I'd think (and even if it's wrong we're merely
generating inttypes.h when we don't have to).

> + /* Tests for macros supposed to be defined in inttypes.h.  */
> + 
> + const char *k = /* implicit string concatenation */
> +   PRId8 PRIi8 PRIo8 PRIu8 PRIx8 PRIX8
> +   PRId16 PRIi16 PRIo16 PRIu16 PRIx16 PRIX16
> +   PRId32 PRIi32 PRIo32 PRIu32 PRIx32 PRIX32

The above test macros also need protecting inside #ifdef.

> + #ifdef INT64_MAX
> +   PRId64 PRIi64
> + #endif

> +   PRIdPTR PRIiPTR PRIoPTR PRIuPTR PRIxPTR PRIXPTR

These need protecting inside ifdef.

Also, since we want to cater to C89 hosts without a 64-bit type, we
need to put macros like PRIdLEAST64 inside an ifdef too, even though
C99 requires these macros.  Otherwise our implementation wouldn't pass
our own test.  (This suggests a natural test case, I suppose....)


I don't see why the changes to lib/stdint_.h, m4/stdint.m4, and
modules/stdint are needed.  They have an effect only if we use
gnulib's inttypes.h and gnulib's stdint.h.  If the program includes
inttypes.h first, this includes stdint.h, which in turn recursively
includes inttypes.h, but this innermost include is a noop because
INTTYPES_H is defined.  A similar argument occurs if the program
includes stdint.h first, with _GL_STDINT_H.  It makes 'configure' run
faster if we omit these changes.

> +   if (!(denom == 0 || (denom == -1 && numer < 0 && - numer < 0)))

This code is attempting to check for undefined behavior, but it in
turn relies on undefined behavior since '- numer' has undefined
behavior when numer < -INTMAX_MAX.  I doubt whether it's worth the
trouble to call 'abort' on undefined behavior, as it's too much pain
to do this portably; we can simply have undefined behavior, as that's
good enough.

Also, the code assumes C99 division behavior, but since gnulib
only assumes C89 the code should defend itself against this.

How about if we use something like the following instead?
(I haven't tested this.)

imaxdiv_t
imaxdiv (intmax_t numer, intmax_t denom)
{
  imaxdiv_t result;

  result.quot = numer / denom;
  result.rem = numer % denom;

  /* C89 allows division to take either the ceiling or the floor if
     either operand is negative, but C99 requires truncation towards
     zero.  Adjust C89's results to match C99, if necessary.  */

  if (result.rem != 0)
    {
      if ((numer < 0) == (denom < 0))
        {
          if ((denom < 0) != (result.rem < 0))
            {
              result.quot--;
              result.rem += denom;
            }
        }
      else
        {
          if ((denom < 0) == (result.rem < 0))
            {
              result.quot++;
              result.rem -= denom;
            }
        }
    }

  return result;
}