Don't use long long where not necessary, some platforms lack it #459
Conversation
|
We are also using long long in tommath.h. You introduced it there yourself in #321 - what shall we do about that? |
Yes, I know I introduced it. There is no guarantee that long long == int64_t, so I don't want to get rid of it. I even would like to add intmax_t and int128_t (switchable, for platforms where it makes sense). But ... in those two places using "long long" is not necessary, it should only be used by the mp_int getters and setters and initters ;-) not in any other places. I don't want to use too many #ifdef's in the code, not more than strictly necessary. ;-) |
|
Ok. Regarding int128 etc - these are not going to happen any time soon here. On platforms which need it, people can add these functions themselves easily. We provide the macros to generate those. But it won't be part of upstream. In contrast to int64_t, a potentially existing int128_t is not part of any c standard. |
|
@nijtmans could you still use SIZEOF_BITS instead of 64 since this makes it easier to grep and replace - the verbosity also helps to show the intent. |
Sure. So you want to write "(int)MP_SIZEOF_BITS(uint64_t)" in stead of "64". Even after the comment one line higher people won't understand it???? If your car-dealer asks you how many wheels your care should have, your answer is "the number of wheels on a 4-wheel car" in stead of simply 4 ??? :-) @sjaeckel, if you have the same opinion as @mindad I'll make the change, but in case you doesn't I don't rush into making this change. |
There are two reasons:
|
|
Well I cannot image someone wanting number_of_wheels(fourwheelcar), I could imagine something like number_of_wheels(biggestcar) or number_of_wheels(heaviestcar) or number_of_wheels(fastedcar). Or ... just describe what you want. What we want is allow the macro's to be useful up to the maximum integral type offered by the platform. Moreover .... this is part of the C standard ... Why don't we use uintmax_t ... Then users can add their own getter/setters for uint128_t or uintmax_t if they want and if their platform supports it. |
|
I do not wwant uintmax since we don't use it anywhere right now and we only support setters up to uint64_t to work without allocation. Please just change it. If someone adds a larger setter, this occurence of uint64_t has to be changed too. If you use uint64_t, this can be found by simple grepping. Maybe also improve the comment saying that it relates to the largest supported setter mp_set_u64 or so. |
|
@minad I'll assign this branch to you to make the changes/comments you want. I'm already spending too much time tweaking it. It doesn't really matter to me, since I'm using my own fork of libtommath anyway ;-( |
|
I understand that we want to add something like Regarding |
btw. I'm not so sure if we really should add that, as it suggests that we're supporting something that we don't test and we're very unlikely to be able to test for some strange platform where it's suddenly 96 or 128bits wide or so. |
I can add a test-case for that, if you want |
please answer my first question first, does it solve a real problem you're having right now? |
|
My goal is this: https://github.com/tcltk/tcl/tree/digit-bit-60. Getting Tcl to work with unmodified libtommath. |
The (minor) goal for this PR is getting rid of direct use of "long long", as it doesn't work with earlier Tcl releases. For Tcl 9 I would like to replace it with intmax_t, to open the road to int128_t in the future. |
that's fine for me! If it's okay for you I'll rebase the PR and only include the |
The getter/setter macro's are written generally. I don't know any platform where intmax_t is more than 64-bits now. intmax_t is defined as the largest integral type available to the compiler, so normally it will be equal to long long. On platforms where long long is 128 and int is 64bit then intmax_t != int64_bit. |
That's OK |
sjaeckel
force-pushed
the
less-long-long
branch
from
ac1a969 to
2846cb9
Compare
|
@nijtmans Do you have a link to the patched version or even better show a diff? Then we could work towards avoiding patching for you. |
Here is the diff :-) https://github.com/libtom/libtommath/compare/support/1.x...tcl-8.7-with-external-libtommath?expand=1. So you can have a look already what you can expect from me in the near future.... This patch assumes that libtommath is built externally, and Tcl is built linking against this external libtommath. It should work even with VC++ 6.0 (but it's not tested yet)!!! |
|
@nijtmans But this is to the 1.x branch? For example NO_STDINT_H has been addressed in develop by the c89<->c99 conversion, so we won't pull that upstream. What changes do you want in 2.x/develop? |
Well, you wanted to see it .. ;-) At the moment, I still use support/1.x as base for Tcl, I will submit my proposals against 2.x, but that will be for Tcl 9.0. I won't use the c89<->c99 conversion, I prefer to provide a replacement stdint.h for platforms having it. But - still then - there are other problems to be addressed due to Tcl's nature. I don't have much hope that libtommath either accepts one of those changes or implements an acceptable alternative solutions. But ... well ... surprise me! :-) |
|
@minad, Anyway I appreciate you at least had a look at the diff and made an attempt to understand the challenges I'm facing. |
|
@nijtmans Ok, I am mostly interested in the 2.x changes. For 1.x I think we will only do small bugfixes.
What are those? I have seen some language runtime usage of tommath before. For example it is being used in moarvm without bigger issues - they use the upstream library afaik. |
sjaeckel
force-pushed
the
less-long-long
branch
from
2846cb9 to
4cbd7d5
Compare
In mtest.c, "long long" is used while "long" suffices. proof: see strtol() call.
Since - according to the comment - "uint64_t can be stored in mp_int without growing", and MP_SIZEOF_BITS(uint64_t) == 64 (by definition), "long long" should be eliminated here too