flintlib / arb

Arb has been merged into FLINT -- use https://github.com/flintlib/flint/ instead
http://arblib.org/
GNU Lesser General Public License v2.1
455 stars 137 forks source link

Use int8_t, int16_t, and int32_t instead of char, short, and int #352

Open jscott0 opened 3 years ago

jscott0 commented 3 years ago

The Arb documentation says

The char, short and int types are assumed to be two’s complement types with exactly 8, 16 and 32 bits. This is not technically guaranteed by the C standard,

This is unnecessary though; the C99 standard says that int8_t, int16_t, and int32_t are defined if and only if they exist, are exact width, and are two's complement. This is more semantic and presumably more portable since it's exactly what you need, and in principle int or the other types could be 64-bit. Since these assumptions have been made about char, short, and int anyway, I don't think this would introduce an ABI break.

Edit: These types are required to exist on POSIX systems. POSIX does not require two's complement or an exact width (only minimum) for char, short, and int.

I realize this pertains more to FLINT but while it's relevant:

Since the C types long and unsigned long do not have a standardized size in practice, FLINT defines slong and ulong types which are guaranteed to be 32 bits on a 32-bit system and 64 bits on a 64-bit system. They are also guaranteed to have the same size as GMP’s mp_limb_t.

I presume this is to ensure that one takes advantage of 64-bit integer computations on Windows. Why not use mp_limb_t if it's intended to be the same?

Usually a limb is implemented as a long. When a long long limb is used this is encoded in the generated gmp.h. This is convenient for applications,

~Alternatively int_fast32_t might get the same job done if it's merely for the sake of speed.~ The int_fastN_t types may not be stable since they're compiler-dependent, but intptr_t ought to work. This appears to be the same as ulong everywhere I've looked. This situation seems complicated relative to its importance, so this can be disregarded unless I can muster up a concrete suggestion.

Thanks for making Arb, Fungrim, and other treasures to come :)

fredrik-johansson commented 3 years ago

I agree that using int8_t etc. would be cleaner. The problem is that we don't use C99. Any changes here should probably be made in Flint first.

fingolfin commented 3 years ago

Just out of curiosity: why aren't you using C99? Even MSVC caved in and added C99 support (for inttypes.h, that happened in Visual Studio 2013. So which systems are you targeting that do not have a C99 compiler?

jscott0 commented 3 years ago

@fingolfin It's to align with FLINT and GMP's code conventions (although there's been discussion of GMP moving to C99/C11 as well). When I raised this issue in FLINT to permit the types, William said (of FLINT)

We don't typically use char, short and int for arithmetic, so changing them seems pointless.

Arb does use these for arithmetic and make assumptions about their internal representations, though, so perhaps this is a worthy Arb-only change. Another advantage of sticking to C89, although moot for this issue, is C++ compatibility.

fingolfin commented 3 years ago

Thank you for clarifying! Two remarks:

  1. While GMP doesn't require C99, it is compatible with it just fine.

  2. While C99 indeed has some changes that are not compatible with C++ (although this got a lot better with C++20), the types like int16_t generally do work there fine ("generally" meaning: I am not aware of a C++ compiler since 2013 which doesn't support them, by including stdint.h and/or inttypes.h).

Case in point, GAP has switch some time ago to require C99; it uses GMP; and some of its files actually are C++ (very simple C++ code as that), and one of the very first thing done in all compilation units is to include stdint.h

(Fun fact: GMP actually also includes stdint.h and/or inttypes.h, if available; it just doesn't require them).

That said: I really don't want to be pushy about this, just wanted to point this out. Thank you again for clarifying the background.