Skip to content

Conversation

@nijtmans
Copy link
Collaborator

title says it all

@nijtmans nijtmans requested review from minad and sjaeckel November 11, 2019 14:57
@minad
Copy link
Member

minad commented Nov 11, 2019

I don't like this. I think the existing getters/setters are more than sufficient. I mean we cannot add such getters/setters for each weird typedef. What about all the others size_t/ssize_t/ptrdiff_t/...?
Furthermore, please show me a relevant platform where uintmax_t is not equal to one of the existing uintx_t types.

@minad
Copy link
Member

minad commented Nov 11, 2019

@nijtmans But please tell me the use case for this. I don't see it. I used intmax_t only very rarely.
I am hoping to reduce the API instead of adding more and more. This is a triviality but still.

@nijtmans
Copy link
Collaborator Author

Furthermore, please show me a relevant platform where uintmax_t is not equal to one of the existing uintx_t types.

At the moment, I don't know any platform where intmax_t is bigger than int64_t. But it's the first candidate to be a 128-bit or even 256-bit type on some super-computers.

I would like in Tcl 9.0 to replace the Tcl_WideInt type with the standard intmax_t, and use the intmax_t functions for all conversions in/out of Tcl. Then we don't need to know whether intmax_t is 32-bit, 64bit or 128bit, it will be future-proof whatever compiler-builders decide to do.

@minad
Copy link
Member

minad commented Nov 11, 2019

Ok, I think you can simply do this via macros. Something along the lines of:

#define mp_get_umax(x) (sizeof (uintmax_t) == sizeof (uint64_t) ? mp_get_u64(x) : mp_get_u32(x)) 

We can add 128/256 types if they become available at some point. Btw I think we can also revert #321 now that we have this c89 mode.

@nijtmans
Copy link
Collaborator Author

What about all the others size_t/ssize_t/ptrdiff_t/...?

I'm not interested in additionaly types like size_t/ssize_t/ptrdiff_t, since I don't have a use-case for that.

@minad
Copy link
Member

minad commented Nov 11, 2019

@nijtmans I am not sure what you are doing in Tcl is not a good idea. It might be better to use precise types. What is the issue of fixing WideInt as 64 bit? Then you get guarantees at the interfaces. I simply don't believe that it will just work on a 256 bit platform if you move to intmax_t. Probably for compatibility reasons intmax_t might even stay 64 bit and int128_t is just added on top. Look at what MS did with the LLP64 mess. But this is speculation and we should not add stuff based on pure speculation if the current API is sufficient.

Furthermore I should add that I don't like this Tcl-driven approach of adding unnecessary stuff to ltm if you can simply do it with a trivial function/macro in the tcl bindings yourself. For most use cases, what we are having is sufficient. If you look what GMP provides, we are already doing much better regarding import/export functions.

@minad
Copy link
Member

minad commented Nov 11, 2019

@nijtmans As a compromise I would agree to adding these as macros if we at the same time revert #321. Otherwise we just have multiple copies of the same function with different names in the library.
Right now we already have two copies since #321.

@nijtmans
Copy link
Collaborator Author

@nijtmans As a compromise I would agree to adding these as macros if we at the same time revert #321.

That's not a compromise, it would make the situation even worse.

@minad
Copy link
Member

minad commented Nov 11, 2019

That's not a compromise, it would make the situation even worse.

Why? long is either 32 or 64 bit? intmax is either 32 or 64 bit? It will work on all platforms with 8 bit bytes. And I doubt that ltm works on dsps with CHAR_BIT!=8.

I close this. Please stop adding code duplication.

@minad minad closed this Nov 11, 2019
@minad
Copy link
Member

minad commented Nov 11, 2019

We already have

  • get_l
  • get_ll
  • set_l
  • set_ll
  • set_ul
  • set_ull
  • init_ul
  • init_ull
  • get_mag_ul
  • get_mag_ull

All of it is unnecessary duplication.

@minad minad deleted the intmax_t branch November 14, 2019 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants