The new approach used in bn_conversion.c makes it impossible to keep no-stdint-h

[karel-m]

Some of you might know about my branch no-stdint-h that avoids ulitising stdint.h which is not available on some compilers.

In v1.1.0 it was as simple as this: https://github.com/libtom/libtommath/commit/602e36ec9ecd97d6af517c462929b523cf26844f

However, since commit https://github.com/libtom/libtommath/commit/3289c959 especially with macro-magic introduced in bn_conversion.c it is not possible anymore to maintain no-stdint-h branch.

I do not agree with changes introduced by https://github.com/libtom/libtommath/pull/285 What exactly was the trouble this PR fixes?

[czurnieden]

What exactly was the trouble this PR fixes?

On point is the lack of signed alternatives, but that is only a minor one. The main problems are the headaches caused by the needs to check if e.g.: and int is 16 bit of 32 bit large, or if the long has 64 bits or only 32, the need to check what of the five possible sizes mp_digit has. Most of the single digit *_d functions have a very restricted usability with low-mp, especially with 8-bit.

That eats a lot of of developing time, needs extended testings, and the necessary maintenance for all of the possible variations, too.

The functions in bn_conversion.c were introduced to get rid of some of the problems listed above which eases maintenance and development with LTM.

We were aware that it comes with the cost of killing backwards-compatibility for some use-cases. Your problem can be solved by taking bn_conversion.c apart such that you can do your search&replace.

I took the liberty to suggest #311 (A quick C&P from #301, hope it went well).

[minad]

@karel-m We (@sjaeckel, @czurnieden and me) decided to use precise types since this increases portability and reduces the maintenance effort. If your compiler does not provide a stdint.h, just add one yourself. It only contains a handful of typedefs. I am using tommath myself in scenarios without libc and I am in exactly the same situation, using my own tiny stdint.h header. Is there an issue which would prevent you from doing that?

[minad]

@nijtmans You mentioned before that tcl avoids stdint.h. Is this still the case?

@karel-m @nijtmans Would something like the following help you? Then you have to provide these sized types yourself.

#ifndef MP_NO_STDINT
typedef uint8_t mp_u8;
typedef uint16_t mp_u16;
typedef uint32_t mp_u32;
typedef uint64_t mp_u64;
typedef int8_t mp_i8;
typedef int16_t mp_i16;
typedef int32_t mp_i32;
typedef int64_t mp_i64;
#endif

[karel-m]

Is there an issue which would prevent you from doing that?

Yes, but let's not waste time, I already lost stdint.h battle.

Something like the following would be absolutely awesome:

#ifndef MP_NO_STDINT
#include <stdint.h>
typedef uint8_t  mp_u8;
typedef uint16_t mp_u16;
typedef uint32_t mp_u32;
typedef uint64_t mp_u64;
typedef int8_t   mp_i8;
typedef int16_t  mp_i16;
typedef int32_t  mp_i32;
typedef int64_t  mp_i64;
#else
typedef unsigned char      mp_u8;
typedef unsigned short     mp_u16;
typedef unsigned int       mp_u32;
typedef unsigned long long mp_u64;
typedef char               mp_i8;
typedef short              mp_i16;
typedef int                mp_i32;
typedef long long          mp_i64;
#endif

[minad]

No, unfortunately this is not an acceptable solution since the type sizes are not fixed. Only the platform or the one porting the library can define these types.

Please elaborate on what the issue with stdint.h is.

[nijtmans]

@nijtmans You mentioned before that tcl avoids stdint.h. Is this still the case?

Yes, that's still the case.

@karel-m @nijtmans Would something like the following help you? Then you have to provide these sized types yourself.

No, I don't think that will help much. I prefer to wait for #311, which is - in my opinion - a step in the right direction. Based on that, I'll compose a proposal for improvements.

The functions previously in bn_conversion.c are not needed when is not available. So, on MSVC 6.0 those functions simply can be left out. But ... I prefer not to shoot at a moving target, I'll simply wait on #311 first.

[minad]

@nijtmans What is the issue of providing a stdint.h yourself on msvc6? We use stdint.h types at multiple places in ltm and this won't change.

[nijtmans]

Well, I'll accept that challenge, after #311 (or something else serving the same purpose) is on "develop" ;-)

[karel-m]

... the type sizes are not fixed

According to my experience, it is much more likely that you meet with a combination of compiler/platform where the stdint.h is missing, incomplete or somehow broken than compiler/platform where char is not 8bit, short is not 16bit, int is not 32bit or long long is not 64bit (although it is not guaranteed; but neither is the presence of stdint.h).

Please elaborate on what the issue with stdint.h is.

Sorry, I am really not interested in this discussion again.

[minad]

@karel-m Please elaborate what the issue is if you provide your own stdint.h.

[nijtmans]

@minad Sorry, but please stop this discussion. You will not convince @karel-m and me, and I will not convince you. We'll have to live with that ....

[minad]

@nijtmans @karel-m Ok. Fact is - stdint.h is used now. So what are you after? I proposed to resolve the situation for systems without the header by using custom mp_u8 types. I am not interested in convincing anyone, I only want to resolve the issue. Please be constructive.

[nijtmans]

I'll help @karel-m to bring something like the no-stdint-h branch back live again. But making it switchable. I'l construct ;-) what I mean after #311 is handled. ;-) I don't think that mp_i8 and the like is a good idea though, there are already standard names for those types that should be used. So I'm against @karel-m 's MP_NO_STDINT piece above, sorry .... Better keep #311 as-is now, and build further from that....

[minad]

@nijtmans Hmm, but how will you handle functions like mp_set_u32? Simply replace uint32_t by unsigned int and hope for the best? If that happens in a separate branch I am ok with it. I would like it if we keep the portable stdint version in develop.

[nijtmans]

@nijtmans Hmm, but how will you handle functions like mp_set_u32?

My answer is: Don't handle it at all. The approach should be similar to #300: If a compiler cannot handle it, just #ifdef out the parts it cannot handle. On MSVC 6.0 it doesn't make sense to provide mp_set_u32 et al.

[minad]

My answer is: Don't handle it at all. The approach should be similar to #300: If a compiler cannot handle it, just #ifdef out the parts it cannot handle. On MSVC 6.0 it doesn't make sense to provide mp_set_u32 et al.

Ok, but there are other functions relying on mp_set_u32 etc. Do you want to ifdef those too? Furthermore this just introduces a lot of needless clutter. If you want to do that in a separate branch, ok. But in develop, please no!

EDIT: Additionally you might then need functions like mp_get_long, which have been deprecated. Those also rely on mp_set_u64 etc. What you are proposing is not feasible.

[nijtmans]

What you are proposing is not feasible.

I expected that answer. Well, I'll give you one more hint: The use-case is build the library with mingw-w64, but include it in a MSVC 6.0 project: So it doesn't matter that functions rely on mp_set_u32, mingw-s64 knows about . The only changes then are needed in tommath.h, those are only minor.

But - again - I stop discussing now on this.

[minad]

@nijtmans Ok, what you are saying is good :+1:

But - again - I stop discussing now on this.

Please don't write things like this. If there is nothing to discuss, we can close this issue in favor of #311 which I will do in a second. #311 should resolve the issue @karel-m and you are having. However trying to stop a technical discussion without exchanging arguments isn't good discussion style in my opinion. This is not a dogmatic religious discussion, but a simple technical matter. For me it would still be interesting to understand why people want to avoid stdint.h (in fact, I thought about that in #285 when @sjaeckel and I were discussing the API).

[karel-m]

Please close this issue once it is possible to apply something like this on develop branch:

git ls-files '*.[ch]' | xargs sed -i -e 's/uint8_t/unsigned char/g' \
                                     -e 's/uint16_t/unsigned short/g' \
                                     -e 's/uint32_t/unsigned int/g' \
                                     -e 's/uint64_t/unsigned long long/g' \
                                     -e 's/int8_t/char/g' \
                                     -e 's/int16_t/short/g' \
                                     -e 's/int32_t/int/g' \
                                     -e 's/int64_t/long long/g' \
                                     -e 's/# *include *<stdint.h>//'

[minad]

@karel-m Has this been resolved to your satisfaction in #313?

[karel-m]

Yes, current remove-bn_conversion branch is ok.

But please do not close this issue until it is merged. I feel that it is still not certain whether #313 will make it to develop or somebody will propose something completely different.

[minad]

But please do not close this issue until it is merged. I feel that it is still not certain whether #313 will make it to develop or somebody will propose something completely different.

I am pretty certain now, that it will. We did it like this multiple times before - resolving an issue by a PR and continuing the discussion in the PR.

[sjaeckel]

I'm pretty certain as well

Gn8

[karel-m]

Nearly there (see no-stdint-h branch), except:

$ make test_standalone
   * cc demo/test.o
demo/test.c: In function ‘test_mp_get_set_i32’:
demo/test.c:168:26: error: ‘INT32_MIN’ undeclared (first use in this function); did you mean ‘INT_MIN’?
    check_get_set_i32(&a, INT32_MIN);
                          ^~~~~~~~~
                          INT_MIN
demo/test.c:168:26: note: each undeclared identifier is reported only once for each function it appears in
demo/test.c:169:26: error: ‘INT32_MAX’ undeclared (first use in this function); did you mean ‘INT32_MIN’?
    check_get_set_i32(&a, INT32_MAX);
                          ^~~~~~~~~
                          INT32_MIN
demo/test.c: In function ‘test_mp_get_set_i64’:
demo/test.c:215:26: error: ‘INT64_MIN’ undeclared (first use in this function); did you mean ‘INT_MIN’?
    check_get_set_i64(&a, INT64_MIN);
                          ^~~~~~~~~
                          INT_MIN
demo/test.c:216:26: error: ‘INT64_MAX’ undeclared (first use in this function); did you mean ‘INT64_MIN’?
    check_get_set_i64(&a, INT64_MAX);
                          ^~~~~~~~~
                          INT64_MIN
make: *** [makefile:23: demo/test.o] Error 1

[minad]

@karel-m you can replace those as well using sed.